Skip to content

warn once when a flipped output transform loses its mirror - #591

Open
fxd0h wants to merge 1 commit into
rustdesk:mainfrom
fxd0h:fix/flipped-transform-warn
Open

warn once when a flipped output transform loses its mirror#591
fxd0h wants to merge 1 commit into
rustdesk:mainfrom
fxd0h:fix/flipped-transform-warn

Conversation

@fxd0h

@fxd0h fxd0h commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

follow-up to a finding on rustdesk/rustdesk#15889: transform_degrees folds the four Flipped variants into their rotation, and the mirror half is dropped before any consumer sees it - so a flipped output ends up streamed mirrored, silently. wayland defines the flipped variants as a vertical-axis mirror followed by the rotation.

this is the interim, not the full fix: the loss is logged once instead of silently, and the doc on the function states exactly what is and is not corrected. the eight-variant mapping test still pins the degree half.

the full fix is carrying the flip bit through WaylandDisplayInfo (serde default keeps old snapshots compatible) and mirroring frames, cursor and hotspot on the consumer side. i verified a producer exists to measure that against - sway accepts output Virtual-1 transform flipped-90 and reports the flipped transform on wl_output - so i plan that as its own change; it touches capture, cursor and input plumbing well beyond this crate.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of flipped display orientations on Wayland.
    • Rotational components are now applied correctly while unsupported mirroring is ignored.
    • Added a one-time warning when a flipped transform is detected.

transform_degrees folds the four Flipped variants into their rotation
and drops the mirror half, so a consumer correcting frames by this
value serves a flipped output mirrored. No compositor of ours can
produce a flipped output to measure the mirror against, so carrying
it waits for a measured producer; until then the loss is said once in
the log instead of silently, and the doc states it.
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f241f81f-1d0d-404e-847f-8cae2ae74a36

📥 Commits

Reviewing files that changed from the base of the PR and between 399030e and addc398.

📒 Files selected for processing (1)
  • src/platform/linux.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

wl_output::Transform handling now maps flipped transforms by rotation, discards their mirror component, and emits a one-time warning. Normal transform mappings remain unchanged.

Changes

Wayland transform handling

Layer / File(s) Summary
Flipped transform mapping
src/platform/linux.rs
Flipped wl_output::Transform variants now map to their rotational components. The code documents the discarded mirror component and emits a single relaxed atomic warning when a flipped transform occurs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to addc3

The PR makes the existing flipped-output limitation visible with a one-time warning without changing current transform behavior. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: logging a one-time warning when a flipped output transform loses its mirror component.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rustdesk

Copy link
Copy Markdown
Owner

I’d request changes on this PR. I found one functional review issue and one documentation issue.

[P2] The new warning is lost on the wayland_probe fallback path

The warning is emitted inside transform_degrees() when a flipped transform is encountered.

But with the wayland_probe feature, collect_wayland_displays() runs inside the probe subprocess. The probe explicitly starts before logging or other startup work, and probe_runtime_dir() calls collect_wayland_displays() there.

That means this sequence occurs:

  1. Parent cannot connect through the normal Wayland environment and starts the probe child.
  2. Child receives Flipped*.
  3. Child calls transform_degrees() and executes log::warn!, but logging has intentionally not been initialized.
  4. Child serializes only the already-lossy degree value.
  5. Parent deserializes that value and returns it; on success it does not receive/re-emit any indication that the original transform was flipped.

So the root/greeter socket-probe path still silently loses the mirror, which defeats the stated purpose of this PR for that path. The PR itself says the interim goal is to make the loss logged rather than silent. ([GitHub]1)

Suggested review comment:

[P2] Preserve the flipped warning across the probe subprocess

transform_degrees() also runs inside probe_runtime_dir() via collect_wayland_displays(), but that child is deliberately dispatched before logging is initialized. Therefore this log::warn! is lost on the wayland_probe fallback path. The parent receives only the serialized degree value, so after deserialization it can no longer tell Flipped90 from _90 and cannot issue the warning itself.

Please carry an is_flipped/full-transform indication through the probe response, or otherwise send a structured diagnostic that the parent can log once. A test covering a flipped transform through the probe protocol would prevent this from regressing.

[P3] The new comment contradicts the PR description

The changed comment says carrying the mirror must wait because there is no measured producer. But the edited PR description says Sway was verified to accept transform flipped-90 and report the flipped transform through wl_output, specifically providing a producer to measure against. ([GitHub]1)

I’d change the rationale to simply say mirror propagation is deferred because it requires changing the downstream data model/capture/cursor/input pipeline. Wayland itself clearly specifies flipped variants as an initial vertical-axis flip followed by rotation. ([Docs.rs]2)

Apart from those, the actual degree mapping is unchanged by this PR, and the relaxed atomic swap is sufficient for the intended process-local “warn at most once” behavior. I didn’t find another correctness regression in the one-file diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants