fix(drm): calibrate the cursor hotspot on drivers without HOTSPOT_X/Y - #16122

sequenceDiagram
  participant PeerInput
  participant DRMProducer
  participant DRMIPC
  participant DRMCapturer
  participant CursorOutput
  PeerInput->>DRMProducer: inject absolute pointer position
  DRMProducer->>DRMIPC: send cursor_pos and hotspot provenance
  DRMIPC->>DRMCapturer: update cursor calibration state
  DRMCapturer->>DRMCapturer: measure and confirm guessed hotspot
  DRMCapturer->>CursorOutput: redeliver corrected cursor shape

Merge Risk: 🟡 Moderate · up to 32ddf

Cursor calibration can publish or retain an incorrect hotspot in supported DRM capture paths, including overriding a kernel-provided top-left hotspot. These correctness issues should be fixed before merge.

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 help to get the list of available commands.}

Reviewed 5beafc6

Request changes for the stale-input and cache-recovery bugs. Both warrant small fixes. The incomplete-snapshot case and test isolation are lower priority; I would not block merging on either alone.

The three cursor findings concern DRM calibration of guessed hotspots. Hidden cursors and hotspots identified as kernel-provided are excluded by should_calibrate() at drm_capturer.rs:714. The likelihood assessments below come from the required code paths, not field reports.

Findings

1. [P2] Invalidate the absolute sample when a relative move is injected (input_service.rs:1013)

   The separate absolute-position tracker keeps the previous absolute position after relative input moves the pointer. Production writes LATEST_PEER_ABS_POS at line 1197, but the relative-move arm at lines 1210-1231 neither updates nor invalidates it. For up to ten seconds, calibration can therefore use a point that is no longer current. The clear helper at line 1003 is test-only.

   With a visible 32×32 cursor whose hotspot is (12,12), send an absolute move to (500,300), then a relative move that shifts the pointer six pixels right. The plane moves from (488,288) to (494,288). Once it settles, the old absolute point produces (6,12), which passes the age and bitmap bounds and is published. A further two-pixel move produces (4,12); that agrees within the two-pixel tolerance and enters CURSOR_CAL_CACHE. The reproduction still reaches both writes at this revision. Later appearances of the shape are seeded with that wrong hotspot at drm_capturer.rs:1443-1460, including after relative input has stopped.

   In practice, this needs a recent absolute event, small relative movements with pauses, and a hardware cursor that stays visible on the host. Relative mode is off by default (input_model.dart:464), and desktop Flutter hides its local cursor while that mode is active (remote_page.dart:1130). That does not stop server calibration: the producer samples the host cursor each tick and includes its position in both frame paths (ipc/drm.rs:1053-1093). I would expect the visible problem mainly after returning to normal input or reusing the cached shape. Ordinary absolute movement does not trigger this sequence.

   Clear the absolute sample before injecting relative movement and wait for another absolute event. Do not substitute the X11 result from get_cursor_pos(). Cover absolute input followed by relative moves and idle frames through the input handler; the old point must not produce a correction.

2. [P2] Remove a cached correction when measurements confirm the original hotspot (drm_capturer.rs:806)

   if confirms && !near_wire prevents unnecessary cache entries when the reader's original hotspot was correct. It also prevents an existing wrong entry from being removed when reliable measurements establish that the original hotspot is correct. Updating c.applied repairs the current cursor, but leaves the stored correction intact. The receive path seeds the next instance from that entry at lines 1443-1460 and publishes it again.

   Seed (25,12) for a shape whose original and true hotspot is (12,12). Two absolute moves to distinct positions both measure (12,12): the current cursor recovers, but the cache still contains (25,12). Receiving the shape again restores the wrong hotspot. The unchanged Rust calibration and publication helpers reproduce this sequence; a control where the confirmed answer differs from the original hotspot correctly replaces the cache entry.

   A further check starts with an empty cache and combines both findings: the relative moves create (4,12), two later absolute measurements restore the current cursor to (12,12), and the next appearance of the shape publishes (4,12) again. No bad cache entry is inserted by the test setup.

   This stays dormant until a bad entry exists. Once it exists, ordinary shape changes, such as arrow → text cursor → arrow, bring the offset back despite successful recalibration. I missed that distinction in the earlier review, which checked the current cursor's recovery without following the stored correction through shape reuse.

   On confirmation, remove the entry when near_wire is true; otherwise store the correction. Extend the existing near-wire test with a pre-existing wrong entry, two valid measurements, and another appearance of the same shape.

3. [P3] Decline calibration when the Wayland snapshot is incomplete (drm_capturer.rs:205)

   When DRM reports multiple outputs but Wayland reports only one, raw_output_transform() returns 0 before checking the matched output's rotation. cal_rect_and_size() has no corresponding rejection: it can still find an exact connector match at line 698 and return that output's rectangle. A rotated output then passes the transform != 0 gate and reaches the subtraction that the gate was meant to prevent.

   In the software-180 fixture, DRM lists two connectors and Wayland lists one matching 1920×1080 output. An injected point (964,544) and plane origin (943,523) produce (21,21) instead of the true (12,12). The same fixture with one DRM connector returns transform 180 and declines. The supplied Python example uses a neighbouring point and produces (23,23) for the same reason.

   I would treat this as a rare edge case: it needs incomplete enumeration, a matching rotated output, and coordinates that still pass the bitmap bounds. In this 180-degree example, that last condition restricts the pointer to a small area near the display centre. Complete snapshots already reject rotation correctly. The code defect is reproducible, but these checks do not establish that users encounter this topology regularly.

   A matching rejection in cal_rect_and_size() is a small fix and avoids changing the existing frame-orientation policy. Cover the partial topology through both lookups; passing 180 directly to note_cursor_plane() misses the conversion to 0.

4. [P3] Serialize the gate tests' shared input fixture (drm_capturer.rs:467)

   Both gate tests now seed LATEST_PEER_ABS_POS, run a measurement, and clear the same global. The mutex protects each access, but the full sequence is not serialized. The rotation test can clear the position at line 494 after the drift test seeds it at line 467 and before it calls note_cursor_plane() at line 469. That attempt then returns at the input-age gate, and both assertions pass without reaching the drift check. The reverse ordering can hide a missing rotation check too.

   Removing the relevant guard makes each test fail alone. Explicitly interleaving the other test's clear between seed and measurement makes it pass with the guard still deleted. This demonstrates a possible false pass, not how often the test runner hits it. It has no direct runtime effect on users and is non-blocking.

   Hold a shared test lock across setup, measurement, and cleanup in both tests, or give each test its own input and drift state. The existing CACHE_TEST_LOCK only covers the separate calibration-cache tests.

fixed at 32ddf83, one commit, each guard with a test that fails when the guard is removed.

1. the relative-move arm clears the absolute sample before it injects the delta, and the internal mouse-move that has no peer position to offer clears it before moving too. the stale point can no longer be measured, and the test drives absolute then relative then idle frames: the old point produces no correction.
2. a near-wire confirmation now removes the cache entry, not just skips the write, so a wrong correction cannot come back on the next appearance of the shape. the cache decision moved into one function the receive loop and the test both run; the test seeds a wrong entry, measures the truth twice, and checks the entry is gone and the reappearance is clean.
3. cal_rect_from declines the partial snapshot the same way raw_output_transform does, both through one predicate. the transform returns an unknown sentinel and the rect returns none. the test covers both lookups with two drm connectors and one rotated wayland output, and drives a full attempt that measures at 0 and declines at the sentinel.
4. the gate tests hold one lock across seed, measurement and clear.
   measured on i915 + mutter: the ew-resize glyph calibrates to the same hotspot before and after this commit, same bbox, same session, so the refactor did not move the live path.

Review

I would request changes before merging this PR.

The overall approach looks sound, and the earlier issues around 180° rotation, layout drift, blocking Wayland enumeration, stale relative-input state, and stale calibration cache entries appear to have been addressed. I still see two correctness issues that should be fixed before merge, plus one calibration-policy issue that I strongly recommend tightening.

1. [P2] Do not infer hotspot-property presence from (hot_x, hot_y) != (0, 0)

hot_from_property is currently derived from:

let hot_from_property = c.hot_x != 0 || c.hot_y != 0;

This conflates two different states:

• HOTSPOT_X/Y properties are absent.
• The properties are present and the real kernel-provided hotspot is (0, 0).

(0, 0) is a perfectly valid hotspot.

The important invariant in this PR is that a kernel-provided hotspot must never be overridden by calibration. With the current representation, a real (0, 0) kernel hotspot becomes hot_from_property = false and is therefore eligible for calibration.

The comment says that this is harmless because measuring such a cursor should produce (0, 0) again, but the rest of the calibration code explicitly accounts for quantization, timing races, and local pointer movement. There is no guarantee that the measured value will be exactly (0, 0). A measurement such as (1, 0) or (2, 1) can still be in-bounds and can eventually be published/cached.

The presence information is currently lost in libdrmtap: get_property_value() itself distinguishes "not found" from a property whose value is zero, but the result is discarded when reading HOTSPOT_X/Y, and drmtap_cursor_info exposes only the values.

I think property presence needs to be carried explicitly from libdrmtap instead of inferred from the coordinates.

Please also add a regression test for:

HOTSPOT property present
hotspot = (0, 0)
=> never eligible for calibration

Be careful with the libdrmtap ABI here. Since RustDesk loads the library dynamically, blindly extending the existing C struct can break compatibility with older .so versions. A versioned/new API or another explicitly ABI-compatible validity channel would be safer.

2. [P2] Confirmation retains pending, but the side effects use raw h

cal_outcome() correctly treats a second measurement within tolerance as confirmation of the existing candidate:

let candidate = if confirms { pending } else { Some(h) };

However, record_measurement() then uses the raw second measurement h for:

near_wire
store_cursor_cal(...)
c.applied

and note_cursor_plane() also delivers/remixes the cursor using that raw h.

So the state machine says:

measurement 1 = (12, 12)
measurement 2 = (14, 12)

(14, 12) is close enough to confirm (12, 12)

but the cache and delivered cursor can become (14, 12).

That makes the meaning of "confirmation" inconsistent.

It can also change the near-wire decision. For example:

wire hotspot = (10, 10)
pending       = (12, 10)
new h         = (14, 10)
tolerance     = 2

The second sample confirms (12, 10), but testing near_wire against (14, 10) gives a different result than testing the retained candidate (12, 10).

I would compute one accepted value:

let accepted = if confirms {
    pending.expect("confirmation must retain a candidate")
} else {
    h
};

and use accepted consistently for:

near_wire
cache storage/removal
c.applied
remix_cursor_id
deliver_drm_cursor

Please add a test where the confirming measurement differs from the pending candidate by 1-2 px and verify the complete side effects, not only cal_outcome().

3. [P2/P3] Strongly consider confirm-before-publish

The current design requires two agreeing windows before writing a correction into the cross-shape cache, but the first measurement is still published immediately.

That means the confirmation step protects future appearances of the shape, but does not protect the currently displayed cursor from a bad one-off measurement.

Example:

last remote absolute position = (500, 300)
real hotspot                  = (12, 12)
real plane origin             = (488, 288)

The remote user stops moving.

Then a local user moves the physical pointer 5 px to the right while the last remote absolute sample is still inside the 10-second age window:

plane origin = (493, 288)
stale remote point = (500, 300)
measured hotspot = (7, 12)

(7, 12) is still inside a normal cursor bitmap and can pass all of the current gates after the plane settles.

The first such measurement is immediately published, even though it has not been confirmed.

A later agreeing window is required only before caching it.

For this bug I would prefer the safer policy:

first window:
    record candidate only

second independent agreeing window:
    publish + cache

The downside is that the first occurrence of a badly guessed cursor may take another pointer stop before being corrected. I think that is preferable to introducing a new occasional incorrect hotspot, especially because this fix is specifically intended to make cursor placement deterministic.

Performance / regression notes

I do not see a serious per-frame allocation regression in the normal successful capture path.

One small regression surface is worth noting: reader.cursor() was moved before grab(). On master, when grab() returns WouldBlock, the loop continues before reading the cursor. With this PR, a full cursor read happens first.

reader.cursor() is not just a property lookup: for a visible cursor it copies the bitmap, allocates colors, walks every pixel, calculates the hash, and calculates the alpha bounding box.

So a stalled/WouldBlock capture now performs cursor work on every retry where master did not.

This is bounded by the existing stall timeout and cursor images are small, so I would not block the PR on it, but it is a real regression surface worth keeping in mind.

There is also an extra raw.clone() for each calibratable new cursor shape so the bitmap can be redelivered after calibration. Again, cursor buffers are small enough that I would not treat this as blocking.

Conclusion

The earlier review findings appear to be addressed, and the general architecture is reasonable.

I would still block merge on #1 and #2.

I also strongly recommend changing #3 to confirm-before-publish, because it makes the calibration semantics much easier to reason about:

unconfirmed measurement => no visible state change
confirmed measurement   => publish and cache

That would give this PR a much stronger correctness invariant and reduce the chance that calibration itself introduces a transient cursor offset.

Code simplification

The current implementation works toward the right goal, but I think the calibration code has become more complicated than necessary.

A large part of the complexity comes from combining:

• lazy Wayland geometry lookup
• output-transform validation
• plane stability tracking
• measurement windows
• first-measurement publishing
• second-measurement confirmation
• cache validation/recovery
• delivered/current hotspot state

I would strongly prefer simplifying the state machine before merging more logic into it.

1. Compute the calibration geometry once when the capturer is created

The capturer already has the DRM display information and the Wayland display snapshot during initialization.

Instead of resolving calibration geometry lazily inside the cursor path, compute a single immutable calibration context up front:

struct CalContext {
    rect: (i32, i32, i32, i32),
    physical_size: (i32, i32),
}

Conceptually:

fn calibration_context(
    drm_displays: &[DrmDisplayInfo],
    wire_idx: usize,
    wayland_displays: &Displays,
) -> Option<CalContext>;

This function should perform all eligibility checks at once:

complete Wayland snapshot
        ↓
matching output
        ↓
supported transform
        ↓
valid logical/physical geometry
        ↓
CalContext

Then the hot cursor path only reads an already validated CalContext.

This should allow removing or significantly simplifying things such as:

cal_transform
raw_output_transform()
cal_rect_and_size()
lazy rect caching
cached_displays() lookup from the cursor path
Option<Option<...>> geometry state

It also gives a stronger invariant: transform and geometry always come from the same display snapshot.

2. Confirm before publishing

The largest source of state-machine complexity is that the first measurement is published immediately, while a second measurement is required for confirmation/cache.

That forces the code to distinguish:

wire hotspot
applied hotspot
pending hotspot
new measurement
confirmed candidate
publish decision
cache decision

I think this should be reduced to:

measurement #1:
    candidate = h
    no visible change

measurement #2:
    if near(candidate, h):
        accepted = candidate
        publish/cache accepted
    else:
        candidate = h
        wait again

Then cal_outcome() is probably unnecessary.

More importantly, there is only one value after confirmation:

accepted

and that exact value is used for:

near-wire comparison
cache insertion/removal
current hotspot
cursor ID remix
cursor delivery

This removes a lot of subtle distinction between pending, h, and applied.

3. Make the hotspot state explicit

Instead of:

applied: Option<(i32, i32)>,
wire_hot: (i32, i32),
pending: Option<(i32, i32)>,

I think the state would be easier to reason about as:

wire_hot: Hotspot,
current_hot: Hotspot,
candidate: Option<Hotspot>,

Initialization becomes:

current_hot = cached_cursor_cal(id).unwrap_or(wire_hot);

The semantics are then obvious:

wire_hot    = what the producer reported
current_hot = what we are currently rendering
candidate   = an unconfirmed measurement

applied == None currently has an implicit meaning that the wire hotspot is active. Making current_hot explicit avoids that hidden state.

4. stable and window can probably be reduced to one simpler lifetime model

The current logic tracks both stability and a measurement window.

I think it can be expressed more directly as:

plane: Option<Pos>,
stable_ticks: u32,
measured_this_position: bool,

When the plane moves:

plane = Some(new_pos);
stable_ticks = 0;
measured_this_position = false;

When it stays still:

stable_ticks += 1;

Then:

if measured_this_position {
    return;
}

if stable_ticks < CURSOR_CAL_STABLE_TICKS {
    return;
}

if stable_ticks > CURSOR_CAL_WINDOW_TICKS {
    return;
}

After attempting the measurement:

measured_this_position = true;

The invariant becomes very simple:

Each stable cursor-plane position produces at most one calibration measurement.

That is easier to understand than maintaining a separate decrementing window state.

5. Separate measurement from policy

note_cursor_plane() currently has too many responsibilities.

I would prefer two very small operations.

Pure geometry:

fn measure_hotspot(
    ctx: &CalContext,
    injected: Pos,
    plane: Pos,
    cursor_size: Size,
) -> Option<Hotspot>;

This only performs coordinate conversion, subtraction, and bounds checking.

And confirmation policy:

fn observe_measurement(
    cal: &mut CursorCal,
    measured: Hotspot,
) -> Option<Hotspot>;

This only implements:

no candidate
    => remember measurement

candidate agrees
    => return confirmed hotspot

candidate disagrees
    => replace candidate

Then the high-level flow becomes approximately:

update_plane_stability();

if !ready_to_measure() {
    return;
}

let Some(measured) = measure_hotspot(...) else {
    return;
};

let Some(accepted) = observe_measurement(cal, measured) else {
    return;
};

apply_confirmed_hotspot(cal, accepted);

That is much easier to audit.

6. Keep the global cache, but keep it dumb

I would keep the cursor calibration cache because recurring shapes such as arrows, resize cursors, and I-beams should not need to recalibrate every time they reappear.

But the cache should only contain:

cursor ID -> confirmed hotspot

It should not participate in the candidate-confirmation state machine.

A cached hotspot can simply initialize current_hot.

Suggested target state

Ideally the per-cursor state ends up close to:

struct CursorCal {
    id: u64,
    raw: CursorShape,

    wire_hot: Hotspot,
    current_hot: Hotspot,
    candidate: Option<Hotspot>,

    plane: Option<Pos>,
    stable_ticks: u32,
    measured_this_position: bool,
}

with an immutable capturer-level:

struct CalContext {
    rect: Rect,
    physical_size: Size,
}

That removes several currently interacting concepts:

applied
window
lazy geometry
Option<Option<rect>>
runtime display-cache lookup
separate calibration transform state
publish-before-confirm behavior

I think this is worth doing because this PR has already grown quite large for what is conceptually a cursor-hotspot correction.

The main goal should be to make the calibration invariant easy to state:

Only a confirmed measurement can change the visible hotspot.

Once that is true, both the implementation and the regression tests become much simpler.