[Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events
Victor Toso
victortoso at redhat.com
Wed May 30 12:14:05 UTC 2018
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 <free.user.name at ya.ru>
> >
> > 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_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.
> >
>
> The lock/unlock is done below, see resulting code
>
> > > 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...
> >
>
> agreed
>
> > > + } 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
> >
>
> I think the test is more similar to previous one, if values change we
> need to do something. Personally I prefer the original proposal.
Ok
>
> > > + if (GetTickCount() - _input_time > VD_INPUT_INTERVAL_MS) {
> > > + return send_input();
> >
> > Another return.. etc.
> >
>
> agreed
>
> > > + } 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;
>
> Patch still apply and works correctly.
> I would say ack, the main objection is the name/e-mail issue.
> The patch also looks long, mainly a lot of variable changes due to
> the "_" member prefix.
>
> Frediano
Feel free to push it with agreed changes or submit a new version
if feel like another iteration is needed.
Cheers,
toso
-------------- 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/20180530/8eddab24/attachment-0001.sig>
More information about the Spice-devel
mailing list