drm: address codex review (per-crtc probe, hidden cursor, --package deb)#7
Open
fxd0h wants to merge 4 commits into
Open
drm: address codex review (per-crtc probe, hidden cursor, --package deb)#7fxd0h wants to merge 4 commits into
fxd0h wants to merge 4 commits into
Conversation
adds an opt-in `drm` feature that captures below the compositor through the privileged libdrmtap helper, for unattended remote access on Wayland: no xdg-desktop-portal consent dialog, and it works at the login screen. off by default. when the feature is off the build is unchanged. everything is gated behind feature = "drm" or lives only in the separate rustdesk-unattended-wayland deb, whose package name is the informed consent. - scrap: DRM/KMS capture backend on libdrmtap-sys, homogeneous per-session backend selection, availability decided by a real grab probe with per-crtc fallback to pipewire/portal - helper: shipped only in the drm deb, installed 0750 root:rustdesk-capture with cap_sys_admin via setcap (postinst appended by build.py) - ci: builds a separate rustdesk-unattended-wayland deb on x86_64 - cursor: downscale the hardware cursor to the served display logical scale - DRM_CAPTURE_SECURITY.md: threat model and hardening notes
- capture_available(): a per-crtc grab failure on the probed crtc 0 (unsupported format, or ENODEV asleep) no longer sinks the whole session to PipeWire; the helper is known to run, so return available and let Display::all() enumerate and drop just the bad connector (new ProbeResult::Available) - update_cursor(): publish a distinct empty cursor when the plane reports the cursor hidden (visible == 0), so get_cursor()'s id changes and the client hides the pointer instead of keeping the last shape - build.py build_deb_from_folder(): a --package bundle carrying the helper now gets the rustdesk-unattended-wayland name and the same group/setcap postinst as build_flutter_deb (extracted into append_drm_helper_postinst)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDRM Debian packaging now shares post-install setup for the capture helper. Staged bundles select dedicated package metadata. DRM dependencies use pinned Git revisions, probing distinguishes helper failures from per-CRTC failures, hidden cursors emit a sentinel shape, and probe tests cover errno and topology state handling. ChangesDRM support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildScript
participant DebianPackage
participant DrmTapHelper
BuildScript->>DebianPackage: select package name and libcap2-bin dependency
BuildScript->>DebianPackage: append configure postinst
DebianPackage->>DrmTapHelper: configure group, permissions, and setcap
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.py`:
- Around line 459-467: Update the final .deb rename in the staged DRM build path
to use the local package_name, matching build_flutter_deb(), so
rustdesk-unattended-wayland bundles are emitted with the correct
package-specific filename while stock bundles retain the rustdesk name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5b4879e-fb0b-44c2-b467-3a1a9a2b2ba6
📒 Files selected for processing (2)
build.pylibs/scrap/src/common/drm.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: CodeRabbit / Review
- GitHub Check: run-ci / build rustdesk linux x86_64-unknown-linux-gnu
- GitHub Check: run-ci / build rustdesk android apk armv7-linux-androideabi
- GitHub Check: run-ci / build rustdesk android apk aarch64-linux-android
- GitHub Check: run-ci / x86_64-pc-windows-msvc
- GitHub Check: run-ci / x86_64-apple-darwin
- GitHub Check: run-ci / aarch64-apple-darwin
- GitHub Check: run-ci / build rustdesk android apk x86_64-linux-android
- GitHub Check: run-ci / aarch64-pc-windows-msvc
- GitHub Check: run-ci / i686-pc-windows-msvc (windows-2022)
- GitHub Check: x86_64-unknown-linux-gnu (ubuntu-24.04)
🔇 Additional comments (7)
build.py (1)
334-354: 🩺 Stability & AvailabilityNo issue here.
res/DEBIAN/postinstalready has a shebang, is executable, and does notexit 0before the appended DRM setup, so the helper postinst block will still run as intended.libs/scrap/src/common/drm.rs (6)
49-55: LGTM!
90-96: LGTM!
125-135: LGTM!
258-266: LGTM!
676-692: LGTM!
765-808: LGTM!
build_deb_from_folder set the control Package to rustdesk-unattended-wayland for a staged drm bundle but still renamed the output to rustdesk-<version>.deb, a filename/metadata mismatch that could also collide with a stock bundle. Match build_flutter_deb and rename by package_name.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Point the drm feature's libdrmtap-sys at the rustdesk-org/libdrmtap fork (pinned by rev to the 0.4.5 / HDR-guard commit df530502) instead of the crates.io crate, so the privileged drmtap-helper is compiled from source RustDesk owns and reviews, in RustDesk CI. Same code as the =0.4.5 pin.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 181-184: Centralize the duplicated libdrmtap-sys git revision by
defining it once under the workspace dependencies in Cargo.toml, then update the
libs/scrap dependency declaration to reference the workspace entry instead of
repeating the git URL and rev; preserve its optional behavior and ensure no
duplicate pin remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f5538b8-2e1e-41a5-a99f-14e08712517f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomllibs/scrap/Cargo.toml
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CodeRabbit / Review
- GitHub Check: run-ci / generate-bridge / generate_bridge (x86_64-unknown-linux-gnu, ubuntu-22.04, 3.22.3, bridge-artifact)
- GitHub Check: run-ci / generate-bridge / generate_bridge (x86_64-unknown-linux-gnu, ubuntu-22.04, 3.44.0, bridge-artifact-flutter-3.44)
- GitHub Check: run-ci / i686-pc-windows-msvc (windows-2022)
- GitHub Check: x86_64-unknown-linux-gnu (ubuntu-24.04)
🔇 Additional comments (2)
libs/scrap/Cargo.toml (1)
58-60: The rev here (df530502...) matches the rootCargo.toml, so Cargo will unify to a single source. The drift/centralization concern is already raised on the root manifest.Cargo.toml (1)
184-184: 🗄️ Data Integrity & IntegrationNo issue: the pinned commit exists and the crate is present in the repository.
> Likely an incorrect or invalid review comment.
fxd0h
force-pushed
the
feature/drm-capture-clean
branch
from
e54377c to
a3399ca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codex flagged 3 P2s on the drm feature (multi-monitor probe, cursor visibility, and the --package deb path). This addresses all three. Opening here first so coderabbit takes a pass before it goes to rustdesk#15420.
verified on a real 3-monitor DRM host: injecting ENOTSUP at the probe keeps all 3 displays on DRM, injecting EACCES falls back; forcing visible==0 flips the cursor id to the hidden sentinel. added a unit test for the errno classification and the bad-crtc set.