Re: [Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
Hi, On Wed, May 30, 2018 at 08:04:21AM -0400, Frediano Ziglio wrote: > > 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 > > > > Would it be possible to use a real name? > > Unfortunately this e-mail was not replied. A signed off with a > dummy name and e-mail does not make sense. I would replace with > a "Patch sent my anonymous with a dummy Signed-off" or > something like that. I would just remove the signed-off but keep the author name. > The patch is solving a real problem, if you increase > VD_INPUT_INTERVAL_MS value (like 1000) is easy to see, move > events are kind of lost. Right, that could be a good addition to the commit log "An easy way to reproduce the problem is by ..." > > > 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. > > > > With the original interval value is hard to spot, see above. > Surely there must be a reason why was easy for the author to > reproduce without changing that value. > > > > > > > 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)); > > Maybe would be time to update these style? > > > > @@ -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_
Re: [Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
> > 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 > > Would it be possible to use a real name? > Unfortunately this e-mail was not replied. A signed off with a dummy name and e-mail does not make sense. I would replace with a "Patch sent my anonymous with a dummy Signed-off" or something like that. The patch is solving a real problem, if you increase VD_INPUT_INTERVAL_MS value (like 1000) is easy to see, move events are kind of lost. > > --- > > 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. > With the original interval value is hard to spot, see above. Surely there must be a reason why was easy for the author to reproduce without changing that value. > > > > 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)); Maybe would be time to update these style? > > @@ -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 butto
Re: [Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
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 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
Re: [Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
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. Ping. Is Windows vdagent still maintained? ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
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 --- 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. 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(); 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(); -