[ros-dev] [BUG] Utterly incorrect implementation of input device coordinates.
James Tabor
jimtabor.rosdev at gmail.com
Mon Jul 27 02:28:42 CEST 2009
So,
please, when you have time to send a patch to fix this wine code
import will be greatly appreciated!
On Thu, Jul 23, 2009 at 2:54 PM, <codeforfood at pulseforce.com> wrote:
> 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 at 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=
>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
More information about the Ros-dev
mailing list