Update pipewire to 0.9 #2448
Update pipewire to 0.9 #2448
Conversation
|
Warning Rate limit exceeded@RobbieTheWagner has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughSwitched the Rust magnifier to a git-pinned pipewire-rs binding, migrated code to the PipeWire 0.9 API, added a build-time cargo-patch flow and libspa patch, enabled Wayland in CI Rust builds, and installed PipeWire system packages in the GitHub Actions workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs (1)
186-195:Stream::new()does not exist in PipeWire 0.9; useStreamBox::new()instead.The PipeWire 0.9 API requires creating a stream with
StreamBox::new(), notStream::new(). After creating the stream, you must call.connect()separately with the appropriate direction, ID, flags, and parameters.Change to:
let stream = pw::stream::StreamBox::new( &core, "swach-screenshot", pw::properties::properties! { *pw::keys::MEDIA_TYPE => "Video", *pw::keys::MEDIA_CATEGORY => "Capture", *pw::keys::MEDIA_ROLE => "Screen", }, ).map_err(|_| "Failed to create PipeWire stream".to_string())?; stream.connect(spa::Direction::Output, None, pw::stream::StreamFlags::empty(), &mut [])?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/electron.yml(1 hunks)electron-app/magnifier/rust-sampler/Cargo.toml(1 hunks)electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rust Tests
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs
[error] 309-309: failed to resolve: could not find Timeout in loop_
[error] 175-175: function or associated item not found in MainLoop: expected new
[error] 179-179: function or associated item not found in pipewire::context::Context: expected new
[error] 187-187: function or associated item not found in pw::stream::Stream: expected new
[error] 309-309: E0433: could not find Timeout in loop_
[error] 175-175: E0599: no function or associated item named new found for struct MainLoop in the current scope
[error] 179-179: E0599: no function or associated item named new found for struct pipewire::context::Context in the current scope
[error] 187-187: E0599: no function or associated item named new found for struct pipewire::stream::Stream in the current scope
[error] cargo test failed for feature set x11,wayland (process completed with exit code 101)
electron-app/magnifier/rust-sampler/Cargo.toml
[error] cargo test failed for feature set x11,wayland (process completed with exit code 101)
⏰ 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: Tests
🔇 Additional comments (3)
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs (1)
221-221: LGTM: VideoInfoRaw construction updated.The change from
VideoInfoRaw::new()toVideoInfoRaw::default()appears correct as there's no compilation error for this line..github/workflows/electron.yml (1)
25-25: LGTM: PipeWire development headers added to CI.Adding
libpipewire-0.3-devcorrectly provides the necessary system libraries for building the Rust sampler with Wayland/PipeWire support.package.json (1)
24-24: LGTM: Build script correctly enables wayland feature.The addition of the
waylandfeature to the cargo build is correct. However, the build will fail until the PipeWire 0.9 API issues inwayland_portal.rsare resolved.Note: This change depends on fixing the compilation errors in the Rust code that uses the PipeWire API.
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
electron-app/magnifier/rust-sampler/PATCHES.md (1)
30-30: Optional: Consider "resolves" for a more formal tone.The word "fixes" is perfectly acceptable here. If you prefer a more formal style, you could use "resolves" instead, but this is purely a stylistic preference.
electron-app/magnifier/rust-sampler/build.rs (1)
1-10: Consider watching the patch file explicitly.The build script correctly applies patches and fails fast on errors. However, line 4 watches the
patches/directory, which may not trigger rebuilds when patch file content changes—only when files are added or removed. Consider adding an explicit watch for the patch file itself to ensure rebuilds when patches are modified.Apply this diff to watch the patch file explicitly:
fn main() { // Ensure build script re-runs if Cargo.toml or patches change println!("cargo:rerun-if-changed=Cargo.toml"); println!("cargo:rerun-if-changed=patches/"); + println!("cargo:rerun-if-changed=patches/libspa-modifier-i64.patch"); // Apply patches to dependencies // This patches the libspa crate to fix the modifier type mismatch // where newer PipeWire versions use i64 instead of u64 cargo_patch::patch().expect("Failed to apply patches to dependencies"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
electron-app/magnifier/rust-sampler/Cargo.toml(1 hunks)electron-app/magnifier/rust-sampler/PATCHES.md(1 hunks)electron-app/magnifier/rust-sampler/build.rs(1 hunks)electron-app/magnifier/rust-sampler/patches/libspa-modifier-i64.patch(1 hunks)electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs (7)
electron-app/magnifier/rust-sampler/src/sampler/macos.rs (1)
new(29-46)electron-app/magnifier/rust-sampler/src/sampler/linux.rs (1)
new(36-63)electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
new(11-23)electron-app/magnifier/rust-sampler/src/types.rs (1)
new(12-18)electron-app/magnifier/rust-sampler/tests/linux_sampler_tests.rs (1)
new(16-22)electron-app/magnifier/rust-sampler/tests/macos_sampler_tests.rs (1)
new(19-25)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
new(15-20)
🪛 LanguageTool
electron-app/magnifier/rust-sampler/PATCHES.md
[style] ~30-~30: Consider using a different verb for a more formal wording.
Context: ... on the PipeWire version. Once upstream fixes this issue, we can remove this patch. ...
(FIX_RESOLVE)
⏰ 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: Tests
🔇 Additional comments (7)
electron-app/magnifier/rust-sampler/PATCHES.md (1)
1-71: Excellent documentation for patch management.The documentation is comprehensive, well-structured, and provides clear guidance for understanding and maintaining the patch system. It covers the rationale, implementation details, troubleshooting, and future maintenance considerations.
electron-app/magnifier/rust-sampler/patches/libspa-modifier-i64.patch (1)
1-18: Patch correctly handles modifier type compatibility.The bidirectional casting approach (u64 ↔ i64) maintains API compatibility while adapting to PipeWire's underlying type change. This is a reasonable workaround. Note that casting between signed and unsigned types could theoretically cause issues if negative values are ever used, but in practice video/DRM modifiers are positive values, making this safe for the current use case.
electron-app/magnifier/rust-sampler/Cargo.toml (2)
44-47: The git commit is legitimate and verified to exist.The pinned commit
b23847892822352f4a98b1cbc61746e9160ad010has been confirmed in the pipewire-rs repository. It was authored by dimtpap on 2025-04-14 and includes FFI-related changes ("spa: Add newtype for node commands") with libspa-sys present in the commit, confirming it contains the necessary FFI bindings.
54-58: Patch remains necessary.The patch configuration is correct and aligns with the build script. No upstream fixes for this libspa Modifier type issue were found in recent releases or commits, so the patch should be retained.
electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs (3)
174-195: PipeWire 0.9 Rc/Box setup matches the documented patternsThe switch to
MainLoopRc::new(None),ContextRc::new(&mainloop, None),connect_rc(None), andStreamBox::new(&core, "swach-screenshot", properties! { ... })lines up with the 0.9.x crate examples for Rc-based main loop/context/core wiring and stream creation, so this migration looks correct. (pipewire.pages.freedesktop.org)If you haven’t already, please confirm CI/cargo check passes against the exact git-pinned
pipewirecommit used here, since those APIs are version-sensitive.
221-222:VideoInfoRaw::default()is fine and equivalent here
VideoInfoRawimplements bothnew()andDefault, and you immediately callparse(param)before reading width/height/stride, so usingVideoInfoRaw::default()is behaviorally equivalent and consistent with the libspa bindings. (pipewire.pages.freedesktop.org)Please just double-check that the pinned
libspa/pipewire-rsversion still exposesDefaultforVideoInfoRaw(it does in 0.9.2).
309-310:Loop::iteratetimeout wrapper matches the 0.9TimeoutAPIIn 0.9.x,
Loop::iteratetakes apw::loop_::Timeout, so callingmainloop.loop_().iterate(pw::loop_::Timeout::Finite(Duration::from_millis(10)))is the correct, typed way to provide a finite timeout. (pipewire.pages.freedesktop.org)
This contradicts an earlier (outdated) review that assumed a plainDurationparameter; no change is needed here with the current crate.Verify that your Cargo.toml/patch is actually pulling in
pipewire≥ 0.9 so this signature matches what’s built.
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.