> > 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@xxxxx> > > 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 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. > > + 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@xxxxxxxxxx> > > > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel