Re: [PATCH vdagent-win] vdagent: fix loss of mouse movement events

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

> ---
> 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@xxxxxxxxxx>


>  bool VDAgent::handle_mon_config(VDAgentMonitorsConfig* mon_config, uint32_t port)
>  {
>      VDIChunk* reply_chunk;
> -- 
> 2.15.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]