New subject: [BUG] Utterly incorrect implementation of input device coordinates.

23 Jul 2009

Here's the information required to patch the incorrect implementation.

Abstract:
- SendInput()/NtUserSendInput() broken:
--- Relative hardware mouse input "working", but not well implemented due
to lack of modifying the travel distance using cursor acceleration and
movement speeds (I've provided info on this as well below), input-virtual
vs screen-real coordinates (even in the relative moves), etc. The two
coordinate spaces are not the same thing!
--- Absolute hardware mouse input broken (used by things like
touchscreens), by a complete misunderstanding of how the virtual input
device coordinate space works (it's NOT in ANY way a 1:1 map that's
translatable to the screen, but the function uses it this way everywhere!)
read below for full info since this is the serious problem.
- SetCursorPos() broken (should NOT be using SendInput() since that's
pointless and ineffective and is meant for the hardware input queue, we
already have the desired coordinates and should move the cursor directly;
and even its use of SendInput() is incorrect since it should FIRST convert
the desired SCREEN coordinates to absolute-INPUT-coordinate-space), BUT
since it's using the broken SendInput() (which currently doesn't know that
absolute input coordinates have NOTHING to do with screen coordinates) it
works correctly and will place cursors at the right location. This will
have to be changed when SendInput() is patched; preferrably to a direct
cursor update (elaborated in the recap below, before the log), rather than
the current indirect/slow approach which places a message in the serial
(as in serial-processing, first-come-first-serve) mouse-input-translation
queue.

So to recap:
- Need to improve relative movement (to handle acceleration and mouse
speed modifiers)
- Need to change the code everywhere where you've been thinking that input
coordinates are the same as screen coordinates (most likely only within
SetCursorPos(), and SendInput()/NtUserSendInput())
- Absolute cursor placement inside SendInput() needs to be rewritten to
convert absolute input-space coordinates to absolute screen-space
coordinates (quite an easy change) using the algorithm I provide below,
which is what Windows uses internally.
- SetCursorPos() needs to COMPLETELY stop using SendInput() and instead
manipulate the cursor X/Y struct directly, or equivalent methods, instead
of injecting itself into the input queue. Alternatively, if it keeps using
the input queue, it needs a reverse-algorithm to turn the desired
screen-coordinates into virtual inputspace-coordinates, such as int
mouseinput_abs_x_or_y_pos = (((65536 * screen_x_or_y_pos) /
screen_width_or_height) + 1). The +1 is necessary for proper rounding.
However, I see no reason to use the input queue within this function as
that involves much more work than simply writing the cursor's location
directly (which is what SendInput EVENTUALLY gets around to doing after
LOTS of processing, and saving on cycles/jumps/calls is GOOD ;)).

All details on how to fix these things are in the below log. Let me know
if there's any questions.

Log (most unrelated discussions/events snipped and long breaks between
explaining different details have been separated with "..."):
 19:56   Yewbacca        . I am sure you've implemented SetCursorPos()
incorrectly. Checking your source code for it at cursor.c:280. This
function is SUPPOSED to take a SCREEN X/Y coordinate (ie X=1400, Y=800 on
a 1440x900 screen, and then moves the cursor location directly, without
emulating mouse movement.
 19:57   Yewbacca        . Instead, your version calls SendInput() with
the x/y unmodified, that's awful. SendInput()'s absolute space is a
virtual desktop square that stretches from 0-65535 in each direction and
will not even be remotely close to what the intended destination is.
 19:57   _0_             . sounds like a patch is in order Yewbacca
 19:57   _0_             . :P
 19:58  +encoded         . Yewbacca, file bug report!
 19:58   Yewbacca        . _0_ Well I'd have to know how you're writing
the cursor position inside NtSendUserInput etc.
 19:58   Yewbacca        . Or file a bug report, I'll do that.
 20:01   Yewbacca        . Related to this, Windows actually uses a
slightly unexpected algorithm for its SendInput() absolute-mode
coordinates-to-screen conversion.
 20:01   Yewbacca        . int destx_or_y = (screen_width_or_height *
x_or_y_abs_coord) / 65536
 20:01   Yewbacca        . If you want pixel precise SendInput() that
works as on windows you'd have to verify this. I'll bug report that too.
 20:02  +encoded         . <3 Yewbacca
 20:02   Yewbacca        . :)
 20:02  +encoded         . you can ofcourse see the source code of
NtSendUserInput
 20:03   Yewbacca        . Yeah I was about to check that to make sure
it's not already correct
 20:04  +encoded         . its in \subsystems\win32\win32k\ntuser\input.c
 20:04   Yewbacca        . Ah thanks, that saves a slow grep.
 20:05  +encoded         . wait opps
 20:05                   . garrythefish_
[n=fisher@unaffiliated/garrythefish] has joined #reactos
 20:05  +encoded         . oh well, nvm me
 20:05   garrythefish_   . community choice award today. hope you guys
win. :D
 20:10   Yewbacca        . Okay after brief confusion I noticed that
IntMouseInput() contains the actual logic for mouse-type input to
SendInput(), input.c:1081. Time to check it.
...
 20:14   Yewbacca        . Okay now I'm really confused. I read through
IntMouseInput() from input.c:1081-1333 and unless I missed a call to a
driver-coordinates-to-screen-coordinates somewhere, this function is
miscoded as well. That'd mean that SetCursorPos() would work correctly
since SendInput() would work incorrectly, and that normal SendInput()
calls that expect a 0 to 65535 range would not work at all.
 20:15   Yewbacca        . It really seems to directly write the pointers
for cursor X and Y with the values it has received
 20:15   Yewbacca        . with no conversion whatsoever
 20:15   Yewbacca        . even though the incoming values are in a 0 to
65535 range
 20:15   Yewbacca        . of that virtual space
...
 20:16   Yewbacca        . Basically for anyone unfamiliar with it, the
driver implements a virtual space for the cursor to either move
relatively within, or absolutely, to allow things like touchscreens to
function easily (they'd just have to SendInput() their current position
as a percentage of the 65535 range).
 20:16   Yewbacca        . So all incoming absolute-mode coordinates must
be converted to screen coordinates, and microsoft does it with int
destx_or_y = (screen_width_or_height * x_or_y_abs_coord) / 65536
 20:17   Yewbacca        . I'm going to re-read it again to see if I
missed some conversion call. If not, that'll be 2 reports. Quite easy
fixes luckily.
 20:18  +encoded         . <3 Yewbacca
 20:18   Yewbacca        . I think the reason you got by this far is
because the HARDWARE mouse sends coordinates relatively
 20:18   Yewbacca        . The problem'd only appear with absolute input
like touchscreens. And since SendInput()'s absolute mode is broken that
means program's SetCursorPos() would work, so it's doubly broken and
would only cause problems for absolute hardware-input (again,
touchscreens mainly).
 20:18   Yewbacca        . :P
 20:19   Yewbacca        . Reading again
 20:20  +encoded         . Yewbacca, we can use someone like you who has
knowledge of windows internals
 20:20   Yewbacca        . Maybe, right now I'm tied up with some major
work :)
...
 20:28   Yewbacca        . Alright I've finished checking everything.
IntMouseInput() stretches from input.c:1081-1333 and is the function
that's called when NtUserSendInput()/SendInput() receives a mouse-type
event. At line 1140 it grabs the pointer to the cursor's X/Y struct with
IntGetCursorLocation(WinSta, &MousePos). Then we get to the significant
errors.
 20:29   Yewbacca        . At lines 1146-1150 it incorrectly writes the
virtual x/y of the INPUT coordinate system right into the memory holding
the SCREEN x/y cursor location
 20:30   Yewbacca        . At lines 1151-1155 it handles relative movement
but doesn't take into account cursor acceleration and movement speed
(those settings we all love in the control panel->mouse), which in turn
can increase the movement distance as much as 4 times the input value.
 20:31   Yewbacca        . Those two behaviours right there would need to
be patched. The first should determine the destination on the screen
through int destx_or_y = (screen_width_or_height * x_or_y_abs_coord) /
65536. The latter should implement handling for cursor acceleration
(check MSDN, it describes acceleration on SendInput()).
 20:32   Yewbacca        . And lastly, SetCursorPos() (forgot which file
it's in) needs to *completely* skip the SendInput() baloney that it's
doing right now (http://pastebin.com/d2db76985) and just directly grab
the cursor X/Y pointer and update it in a simple assignment, AFTER
checking that it's in range of course (otherwise clamping it to the
screen).
 20:33  +Lone_Rifle      . Yewbacca, suggest that you subscribe to ros-dev
and retype or copy-paste your findings there, it's quite a bit of text to
go through in one sitting
 20:33   Yewbacca        . Oh yeah and if you wanna be 100% complete, also
implement a check for ClipCursor() and don't allow the cursor to move
outside that, no matter if it's from hardware input or SetCursorPos(). If
it's outside, it should be clamped to the nearest inside value.
 20:34   Yewbacca        . Lone_Rifle Yeah, I agree and was going to
submit it. I'll do that now.
...
 20:37   Yewbacca        . Acceleration is described at
http://msdn.microsoft.com/en-us/library/ms646273(VS.85).aspx (info for
the MOUSEINPUT struct). I'll submit this log to the mailing list.

=EOF=