[Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
Victor Toso
victortoso at redhat.com
Mon Mar 5 11:43:11 UTC 2018
Hi,
On Fri, Feb 16, 2018 at 03:05:39PM +0300, free.user.name wrote:
> send_input() may not be immediately called from handle_mouse_event() on
> movement. INPUT structure is generated and stored and a timer may be set
> instead. If subsequent call to handle_mouse_event() occurs before timer
> expires, prepared INPUT structure gets overwritten and MOUSEEVENTF_MOVE
> bit is lost. Windows doesn't see updated mouse position as the result.
>
> Make handle_mouse_event() merely store the new mouse state, and move
> INPUT structure generation to send_input(). Shuffle new mouse state to
> previous only after mouse events are submitted to SendInput() Windows
> API function.
>
> Signed-off-by: free.user.name <free.user.name at ya.ru>
Would it be possible to use a real name?
> ---
> This fixes a very annoying mouse input bug in Windows 7/10 guests for
> me. Whenever vdagent-win is handling mouse input, mouse pointer position
> seen by the guest sometimes lags behind the cursor drawn on screen,
> which makes it impossible to reliably click on anything.
How easy is that to reproduce? I couldn't notice any lag but
maybe it is just me and/or my environment.
>
> vdagent/vdagent.cpp | 132 +++++++++++++++++++++++++---------------------------
> 1 file changed, 63 insertions(+), 69 deletions(-)
>
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index f00fbf5..89b3c6a 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -89,8 +89,7 @@ private:
> void on_clipboard_grab();
> void on_clipboard_request(UINT format);
> void on_clipboard_release();
> - DWORD get_buttons_change(DWORD last_buttons_state, DWORD new_buttons_state,
> - DWORD mask, DWORD down_flag, DWORD up_flag);
> + DWORD get_buttons_change(DWORD mask, DWORD down_flag, DWORD up_flag);
> static HGLOBAL utf8_alloc(LPCSTR data, int size);
> static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> static DWORD WINAPI event_thread_proc(LPVOID param);
> @@ -131,10 +130,8 @@ private:
> int _system_version;
> int _clipboard_owner;
> DWORD _clipboard_tick;
> - DWORD _buttons_state;
> - ULONG _mouse_x;
> - ULONG _mouse_y;
> - INPUT _input;
> + VDAgentMouseState _new_mouse;
> + VDAgentMouseState _last_mouse;
> DWORD _input_time;
> HANDLE _control_event;
> HANDLE _stop_event;
> @@ -191,9 +188,6 @@ VDAgent::VDAgent()
> , _remove_clipboard_listener (NULL)
> , _clipboard_owner (owner_none)
> , _clipboard_tick (0)
> - , _buttons_state (0)
> - , _mouse_x (0)
> - , _mouse_y (0)
> , _input_time (0)
> , _control_event (NULL)
> , _stop_event (NULL)
> @@ -221,7 +215,8 @@ VDAgent::VDAgent()
> swprintf_s(log_path, MAX_PATH, VD_AGENT_LOG_PATH, temp_path);
> _log = VDLog::get(log_path);
> }
> - ZeroMemory(&_input, sizeof(_input));
> + ZeroMemory(&_new_mouse, sizeof(_new_mouse));
> + ZeroMemory(&_last_mouse, sizeof(_last_mouse));
> ZeroMemory(&_read_overlapped, sizeof(_read_overlapped));
> ZeroMemory(&_write_overlapped, sizeof(_write_overlapped));
> ZeroMemory(_read_buf, sizeof(_read_buf));
> @@ -522,13 +517,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
> }
> }
>
> -DWORD VDAgent::get_buttons_change(DWORD last_buttons_state, DWORD new_buttons_state,
> - DWORD mask, DWORD down_flag, DWORD up_flag)
> +DWORD VDAgent::get_buttons_change(DWORD mask, DWORD down_flag, DWORD up_flag)
> {
> DWORD ret = 0;
> - if (!(last_buttons_state & mask) && (new_buttons_state & mask)) {
> + if (!(_last_mouse.buttons & mask) && (_new_mouse.buttons & mask)) {
> ret = down_flag;
> - } else if ((last_buttons_state & mask) && !(new_buttons_state & mask)) {
> + } else if ((_last_mouse.buttons & mask) && !(_new_mouse.buttons & mask)) {
> ret = up_flag;
> }
> return ret;
> @@ -536,101 +530,101 @@ DWORD VDAgent::get_buttons_change(DWORD last_buttons_state, DWORD new_buttons_st
>
> bool VDAgent::send_input()
> {
> + DisplayMode* mode = NULL;
> + DWORD mouse_move = 0;
> + DWORD buttons_change = 0;
> + DWORD mouse_wheel = 0;
> bool ret = true;
> - _desktop_layout->lock();
> + INPUT input;
> +
> if (_pending_input) {
> if (KillTimer(_hwnd, VD_TIMER_ID)) {
> _pending_input = false;
> } else {
> vd_printf("KillTimer failed: %lu", GetLastError());
> _running = false;
> - _desktop_layout->unlock();
Did not follow this change.
> return false;
> }
> }
> - if (!SendInput(1, &_input, sizeof(INPUT))) {
> - DWORD err = GetLastError();
> - // Don't stop agent due to UIPI blocking, which is usually only for specific windows
> - // of system security applications (anti-viruses etc.)
> - if (err != ERROR_SUCCESS && err != ERROR_ACCESS_DENIED) {
> - vd_printf("SendInput failed: %lu", err);
> - ret = _running = false;
> - }
> - }
> - _input_time = GetTickCount();
> - _desktop_layout->unlock();
> - return ret;
> -}
> -
> -bool VDAgent::handle_mouse_event(VDAgentMouseState* state)
> -{
> - DisplayMode* mode = NULL;
> - DWORD mouse_move = 0;
> - DWORD buttons_change = 0;
> - DWORD mouse_wheel = 0;
> - bool ret = true;
>
> ASSERT(_desktop_layout);
> _desktop_layout->lock();
> - if (state->display_id < _desktop_layout->get_display_count()) {
> - mode = _desktop_layout->get_display(state->display_id);
> + if (_new_mouse.display_id < _desktop_layout->get_display_count()) {
> + mode = _desktop_layout->get_display(_new_mouse.display_id);
> }
> if (!mode || !mode->get_attached()) {
> _desktop_layout->unlock();
> return true;
> }
> - ZeroMemory(&_input, sizeof(INPUT));
> - _input.type = INPUT_MOUSE;
> - if (state->x != _mouse_x || state->y != _mouse_y) {
> + ZeroMemory(&input, sizeof(INPUT));
> + input.type = INPUT_MOUSE;
> + if (_new_mouse.x != _last_mouse.x || _new_mouse.y != _last_mouse.y) {
> DWORD w = _desktop_layout->get_total_width();
> DWORD h = _desktop_layout->get_total_height();
> w = (w > 1) ? w-1 : 1; /* coordinates are 0..w-1, protect w==0 */
> h = (h > 1) ? h-1 : 1; /* coordinates are 0..h-1, protect h==0 */
> - _mouse_x = state->x;
> - _mouse_y = state->y;
> mouse_move = MOUSEEVENTF_MOVE;
> - _input.mi.dx = (mode->get_pos_x() + _mouse_x) * 0xffff / w;
> - _input.mi.dy = (mode->get_pos_y() + _mouse_y) * 0xffff / h;
> + input.mi.dx = (mode->get_pos_x() + _new_mouse.x) * 0xffff / w;
> + input.mi.dy = (mode->get_pos_y() + _new_mouse.y) * 0xffff / h;
> }
> - if (state->buttons != _buttons_state) {
> - buttons_change = get_buttons_change(_buttons_state, state->buttons, VD_AGENT_LBUTTON_MASK,
> + if (_new_mouse.buttons != _last_mouse.buttons) {
> + buttons_change = get_buttons_change(VD_AGENT_LBUTTON_MASK,
> MOUSEEVENTF_LEFTDOWN, MOUSEEVENTF_LEFTUP) |
> - get_buttons_change(_buttons_state, state->buttons, VD_AGENT_MBUTTON_MASK,
> + get_buttons_change(VD_AGENT_MBUTTON_MASK,
> MOUSEEVENTF_MIDDLEDOWN, MOUSEEVENTF_MIDDLEUP) |
> - get_buttons_change(_buttons_state, state->buttons, VD_AGENT_RBUTTON_MASK,
> + get_buttons_change(VD_AGENT_RBUTTON_MASK,
> MOUSEEVENTF_RIGHTDOWN, MOUSEEVENTF_RIGHTUP);
> - mouse_wheel = get_buttons_change(_buttons_state, state->buttons,
> - VD_AGENT_UBUTTON_MASK | VD_AGENT_DBUTTON_MASK,
> + mouse_wheel = get_buttons_change(VD_AGENT_UBUTTON_MASK | VD_AGENT_DBUTTON_MASK,
> MOUSEEVENTF_WHEEL, 0);
> if (mouse_wheel) {
> - if (state->buttons & VD_AGENT_UBUTTON_MASK) {
> - _input.mi.mouseData = WHEEL_DELTA;
> - } else if (state->buttons & VD_AGENT_DBUTTON_MASK) {
> - _input.mi.mouseData = (DWORD)(-WHEEL_DELTA);
> + if (_new_mouse.buttons & VD_AGENT_UBUTTON_MASK) {
> + input.mi.mouseData = WHEEL_DELTA;
> + } else if (_new_mouse.buttons & VD_AGENT_DBUTTON_MASK) {
> + input.mi.mouseData = (DWORD)(-WHEEL_DELTA);
> }
> }
> - _buttons_state = state->buttons;
> }
>
> - _input.mi.dwFlags = MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_VIRTUALDESK | mouse_move |
> - mouse_wheel | buttons_change;
> + input.mi.dwFlags = MOUSEEVENTF_ABSOLUTE | MOUSEEVENTF_VIRTUALDESK | mouse_move |
> + mouse_wheel | buttons_change;
>
> - if ((mouse_move && GetTickCount() - _input_time > VD_INPUT_INTERVAL_MS) || buttons_change ||
> - mouse_wheel) {
> - ret = send_input();
> - } else if (!_pending_input) {
> - if (SetTimer(_hwnd, VD_TIMER_ID, VD_INPUT_INTERVAL_MS, NULL)) {
> - _pending_input = true;
> - } else {
> - vd_printf("SetTimer failed: %lu", GetLastError());
> - _running = false;
> - ret = false;
> + if (!SendInput(1, &input, sizeof(INPUT))) {
> + DWORD err = GetLastError();
> + // Don't stop agent due to UIPI blocking, which is usually only for specific windows
> + // of system security applications (anti-viruses etc.)
> + if (err != ERROR_SUCCESS && err != ERROR_ACCESS_DENIED) {
> + vd_printf("SendInput failed: %lu", err);
> + ret = _running = false;
> }
> + } else {
> + _last_mouse = _new_mouse;
> }
> + _input_time = GetTickCount();
> _desktop_layout->unlock();
> return ret;
> }
>
> +bool VDAgent::handle_mouse_event(VDAgentMouseState* state)
> +{
> + _new_mouse = *state;
> + if (_new_mouse.buttons != _last_mouse.buttons) {
> + return send_input();
IMHO, I'm all in for early returns instead of several if/else,
so...
> + } else if (_new_mouse.x != _last_mouse.x || _new_mouse.y != _last_mouse.y) {
No else here, I'd switch the logic to return true if
_new.x == _last.x && _new.y == _last.y
> + if (GetTickCount() - _input_time > VD_INPUT_INTERVAL_MS) {
> + return send_input();
Another return.. etc.
> + } else if (!_pending_input) {
> + if (SetTimer(_hwnd, VD_TIMER_ID, VD_INPUT_INTERVAL_MS, NULL)) {
> + _pending_input = true;
> + } else {
> + vd_printf("SetTimer failed: %lu", GetLastError());
> + _running = false;
> + return false;
> + }
> + }
> + }
> + return true;
> +}
> +
I see that patch applies cleanly build and runs without issue. I
could see this as improvement but it would be nice If I could
reproduce the issue that you have, if possible.
Reviewed-by: Victor Toso <victortoso at redhat.com>
> bool VDAgent::handle_mon_config(VDAgentMonitorsConfig* mon_config, uint32_t port)
> {
> VDIChunk* reply_chunk;
> --
> 2.15.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180305/f61e1f14/attachment-0001.sig>
More information about the Spice-devel
mailing list