[PATCH 0/5] MR3686: winewayland.drv: part 6: Handle wl_pointer (mouse) events.
This MR adds support for handling `wl_pointer` devices: 1. Handle enter/leave/(absolute) motion/button/axis events to allow users to interact with applications using such devices, e.g., a mouse. 2. Update the cursor image used for Wayland surfaces based on the Windows cursor bitmap data. A few notes: 1. A single `wl_seat`/`wl_pointer` is supported for now. 2. Many of the input interactions require window management support that hasn't been implemented yet (e.g., popup positioning, resizing, max/fullscreen). 3. There is some potential for bad interactions between the input event dispatching mechanism and the `win32u` flushing logic for `window_surface` , which can lead to visual glitches. Although I don't think this is blocker for this MR, I would like to start a discussion about the problem and mitigation ideas in a comment below. Thanks! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> Track the availability of a pointer device for a seat. For now we assume only a single seat and pointer. --- dlls/winewayland.drv/Makefile.in | 1 + dlls/winewayland.drv/wayland.c | 43 +++++++++++ dlls/winewayland.drv/wayland_pointer.c | 98 ++++++++++++++++++++++++++ dlls/winewayland.drv/waylanddrv.h | 9 +++ 4 files changed, 151 insertions(+) create mode 100644 dlls/winewayland.drv/wayland_pointer.c diff --git a/dlls/winewayland.drv/Makefile.in b/dlls/winewayland.drv/Makefile.in index ef75c289b76..6bc03187d61 100644 --- a/dlls/winewayland.drv/Makefile.in +++ b/dlls/winewayland.drv/Makefile.in @@ -9,6 +9,7 @@ SOURCES = \ version.rc \ wayland.c \ wayland_output.c \ + wayland_pointer.c \ wayland_surface.c \ waylanddrv_main.c \ window.c \ diff --git a/dlls/winewayland.drv/wayland.c b/dlls/winewayland.drv/wayland.c index d35bd061b7e..6ea4a94551d 100644 --- a/dlls/winewayland.drv/wayland.c +++ b/dlls/winewayland.drv/wayland.c @@ -53,6 +53,29 @@ static const struct xdg_wm_base_listener xdg_wm_base_listener = xdg_wm_base_handle_ping }; +/********************************************************************** + * wl_seat handling + */ + +static void wl_seat_handle_capabilities(void *data, struct wl_seat *seat, + enum wl_seat_capability caps) +{ + if ((caps & WL_SEAT_CAPABILITY_POINTER) && !process_wayland.wl_pointer) + wayland_pointer_init(wl_seat_get_pointer(seat)); + else if (!(caps & WL_SEAT_CAPABILITY_POINTER) && process_wayland.wl_pointer) + wayland_pointer_deinit(); +} + +static void wl_seat_handle_name(void *data, struct wl_seat *seat, const char *name) +{ +} + +static const struct wl_seat_listener seat_listener = +{ + wl_seat_handle_capabilities, + wl_seat_handle_name +}; + /********************************************************************** * Registry handling */ @@ -98,6 +121,17 @@ static void registry_handle_global(void *data, struct wl_registry *registry, { process_wayland.wl_shm = wl_registry_bind(registry, id, &wl_shm_interface, 1); } + else if (strcmp(interface, "wl_seat") == 0) + { + if (process_wayland.wl_seat) + { + WARN("Only a single seat is currently supported, ignoring additional seats.\n"); + return; + } + process_wayland.wl_seat = wl_registry_bind(registry, id, &wl_seat_interface, + version < 5 ? version : 5); + wl_seat_add_listener(process_wayland.wl_seat, &seat_listener, NULL); + } } static void registry_handle_global_remove(void *data, struct wl_registry *registry, @@ -116,6 +150,15 @@ static void registry_handle_global_remove(void *data, struct wl_registry *regist return; } } + + if (process_wayland.wl_seat && + wl_proxy_get_id((struct wl_proxy *)process_wayland.wl_seat) == id) + { + TRACE("removing seat\n"); + if (process_wayland.wl_pointer) wayland_pointer_deinit(); + wl_seat_release(process_wayland.wl_seat); + process_wayland.wl_seat = NULL; + } } static const struct wl_registry_listener registry_listener = { diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c new file mode 100644 index 00000000000..10dacfa03b0 --- /dev/null +++ b/dlls/winewayland.drv/wayland_pointer.c @@ -0,0 +1,98 @@ +/* + * Wayland pointer handling + * + * Copyright (c) 2020 Alexandros Frantzis for Collabora Ltd + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep unix +#endif + +#include "config.h" + +#include "waylanddrv.h" + +static void pointer_handle_motion(void *data, struct wl_pointer *wl_pointer, + uint32_t time, wl_fixed_t sx, wl_fixed_t sy) +{ +} + +static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, + uint32_t serial, struct wl_surface *wl_surface, + wl_fixed_t sx, wl_fixed_t sy) +{ +} + +static void pointer_handle_leave(void *data, struct wl_pointer *wl_pointer, + uint32_t serial, struct wl_surface *wl_surface) +{ +} + +static void pointer_handle_button(void *data, struct wl_pointer *wl_pointer, + uint32_t serial, uint32_t time, uint32_t button, + uint32_t state) +{ +} + +static void pointer_handle_axis(void *data, struct wl_pointer *wl_pointer, + uint32_t time, uint32_t axis, wl_fixed_t value) +{ +} + +static void pointer_handle_frame(void *data, struct wl_pointer *wl_pointer) +{ +} + +static void pointer_handle_axis_source(void *data, struct wl_pointer *wl_pointer, + uint32_t axis_source) +{ +} + +static void pointer_handle_axis_stop(void *data, struct wl_pointer *wl_pointer, + uint32_t time, uint32_t axis) +{ +} + +static void pointer_handle_axis_discrete(void *data, struct wl_pointer *wl_pointer, + uint32_t axis, int32_t discrete) +{ +} + +static const struct wl_pointer_listener pointer_listener = +{ + pointer_handle_enter, + pointer_handle_leave, + pointer_handle_motion, + pointer_handle_button, + pointer_handle_axis, + pointer_handle_frame, + pointer_handle_axis_source, + pointer_handle_axis_stop, + pointer_handle_axis_discrete +}; + +void wayland_pointer_init(struct wl_pointer *wl_pointer) +{ + process_wayland.wl_pointer = wl_pointer; + wl_pointer_add_listener(process_wayland.wl_pointer, &pointer_listener, NULL); +} + +void wayland_pointer_deinit(void) +{ + wl_pointer_release(process_wayland.wl_pointer); + process_wayland.wl_pointer = NULL; +} diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 0fd2a7a6c77..8932c2c9e8d 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -66,6 +66,8 @@ struct wayland struct wl_compositor *wl_compositor; struct xdg_wm_base *xdg_wm_base; struct wl_shm *wl_shm; + struct wl_seat *wl_seat; + struct wl_pointer *wl_pointer; struct wl_list output_list; /* Protects the output_list and the wayland_output.current states. */ pthread_mutex_t output_mutex; @@ -167,6 +169,13 @@ void wayland_window_surface_update_wayland_surface(struct window_surface *surfac struct wayland_surface *wayland_surface) DECLSPEC_HIDDEN; void wayland_window_flush(HWND hwnd) DECLSPEC_HIDDEN; +/********************************************************************** + * Wayland pointer + */ + +void wayland_pointer_init(struct wl_pointer *wl_pointer) DECLSPEC_HIDDEN; +void wayland_pointer_deinit(void) DECLSPEC_HIDDEN; + /********************************************************************** * Helpers */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> Handle wl_pointer enter/leave events and maintain information about the focused wayland_surface. Since pointer information will be accessed by any UI capable thread, ensure proper proper locking is in place. --- dlls/winewayland.drv/wayland.c | 7 ++-- dlls/winewayland.drv/wayland_pointer.c | 44 +++++++++++++++++++++++--- dlls/winewayland.drv/wayland_surface.c | 25 +++++++++------ dlls/winewayland.drv/waylanddrv.h | 8 ++++- 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/dlls/winewayland.drv/wayland.c b/dlls/winewayland.drv/wayland.c index 6ea4a94551d..51e36b5ba60 100644 --- a/dlls/winewayland.drv/wayland.c +++ b/dlls/winewayland.drv/wayland.c @@ -34,6 +34,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv); struct wayland process_wayland = { + .pointer.mutex = PTHREAD_MUTEX_INITIALIZER, .output_list = {&process_wayland.output_list, &process_wayland.output_list}, .output_mutex = PTHREAD_MUTEX_INITIALIZER }; @@ -60,9 +61,9 @@ static const struct xdg_wm_base_listener xdg_wm_base_listener = static void wl_seat_handle_capabilities(void *data, struct wl_seat *seat, enum wl_seat_capability caps) { - if ((caps & WL_SEAT_CAPABILITY_POINTER) && !process_wayland.wl_pointer) + if ((caps & WL_SEAT_CAPABILITY_POINTER) && !process_wayland.pointer.wl_pointer) wayland_pointer_init(wl_seat_get_pointer(seat)); - else if (!(caps & WL_SEAT_CAPABILITY_POINTER) && process_wayland.wl_pointer) + else if (!(caps & WL_SEAT_CAPABILITY_POINTER) && process_wayland.pointer.wl_pointer) wayland_pointer_deinit(); } @@ -155,7 +156,7 @@ static void registry_handle_global_remove(void *data, struct wl_registry *regist wl_proxy_get_id((struct wl_proxy *)process_wayland.wl_seat) == id) { TRACE("removing seat\n"); - if (process_wayland.wl_pointer) wayland_pointer_deinit(); + if (process_wayland.pointer.wl_pointer) wayland_pointer_deinit(); wl_seat_release(process_wayland.wl_seat); process_wayland.wl_seat = NULL; } diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c index 10dacfa03b0..1476c2369a9 100644 --- a/dlls/winewayland.drv/wayland_pointer.c +++ b/dlls/winewayland.drv/wayland_pointer.c @@ -25,6 +25,9 @@ #include "config.h" #include "waylanddrv.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv); static void pointer_handle_motion(void *data, struct wl_pointer *wl_pointer, uint32_t time, wl_fixed_t sx, wl_fixed_t sy) @@ -35,11 +38,38 @@ static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, uint32_t serial, struct wl_surface *wl_surface, wl_fixed_t sx, wl_fixed_t sy) { + struct wayland_surface *surface; + + if (!wl_surface) return; + + pthread_mutex_lock(&process_wayland.pointer.mutex); + if ((surface = wayland_surface_lock_proxy((struct wl_proxy *)wl_surface))) + { + TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); + process_wayland.pointer.surface = surface; + pthread_mutex_unlock(&surface->mutex); + } + pthread_mutex_unlock(&process_wayland.pointer.mutex); } static void pointer_handle_leave(void *data, struct wl_pointer *wl_pointer, uint32_t serial, struct wl_surface *wl_surface) { + struct wayland_surface *surface; + + if (!wl_surface) return; + + pthread_mutex_lock(&process_wayland.pointer.mutex); + if ((surface = wayland_surface_lock_proxy((struct wl_proxy *)wl_surface))) + { + if (process_wayland.pointer.surface == surface) + { + TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); + process_wayland.pointer.surface = NULL; + } + pthread_mutex_unlock(&surface->mutex); + } + pthread_mutex_unlock(&process_wayland.pointer.mutex); } static void pointer_handle_button(void *data, struct wl_pointer *wl_pointer, @@ -87,12 +117,18 @@ static const struct wl_pointer_listener pointer_listener = void wayland_pointer_init(struct wl_pointer *wl_pointer) { - process_wayland.wl_pointer = wl_pointer; - wl_pointer_add_listener(process_wayland.wl_pointer, &pointer_listener, NULL); + pthread_mutex_lock(&process_wayland.pointer.mutex); + process_wayland.pointer.wl_pointer = wl_pointer; + process_wayland.pointer.surface = NULL; + pthread_mutex_unlock(&process_wayland.pointer.mutex); + wl_pointer_add_listener(process_wayland.pointer.wl_pointer, &pointer_listener, NULL); } void wayland_pointer_deinit(void) { - wl_pointer_release(process_wayland.wl_pointer); - process_wayland.wl_pointer = NULL; + pthread_mutex_lock(&process_wayland.pointer.mutex); + wl_pointer_release(process_wayland.pointer.wl_pointer); + process_wayland.pointer.wl_pointer = NULL; + process_wayland.pointer.surface = NULL; + pthread_mutex_unlock(&process_wayland.pointer.mutex); } diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index fbc49191d9e..6f637516078 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -33,17 +33,17 @@ WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv); -/* Protects access to the user data of xdg_surface */ -static pthread_mutex_t xdg_data_mutex = PTHREAD_MUTEX_INITIALIZER; +/* Protects access to the wayland_surface user data of wayland objects */ +static pthread_mutex_t user_data_mutex = PTHREAD_MUTEX_INITIALIZER; -static struct wayland_surface *wayland_surface_lock_xdg(struct xdg_surface *xdg_surface) +struct wayland_surface *wayland_surface_lock_proxy(struct wl_proxy *wl_proxy) { struct wayland_surface *surface; - pthread_mutex_lock(&xdg_data_mutex); - surface = xdg_surface_get_user_data(xdg_surface); + pthread_mutex_lock(&user_data_mutex); + surface = wl_proxy_get_user_data(wl_proxy); if (surface) pthread_mutex_lock(&surface->mutex); - pthread_mutex_unlock(&xdg_data_mutex); + pthread_mutex_unlock(&user_data_mutex); return surface; } @@ -57,7 +57,7 @@ static void xdg_surface_handle_configure(void *data, struct xdg_surface *xdg_sur TRACE("serial=%u\n", serial); - if (!(surface = wayland_surface_lock_xdg(xdg_surface))) return; + if (!(surface = wayland_surface_lock_proxy((struct wl_proxy *)xdg_surface))) return; /* Handle this event only if wayland_surface is still associated with * the target xdg_surface. */ @@ -108,6 +108,7 @@ struct wayland_surface *wayland_surface_create(HWND hwnd) ERR("Failed to create wl_surface Wayland surface\n"); goto err; } + wl_surface_set_user_data(surface->wl_surface, surface); return surface; @@ -123,7 +124,7 @@ err: */ void wayland_surface_destroy(struct wayland_surface *surface) { - pthread_mutex_lock(&xdg_data_mutex); + pthread_mutex_lock(&user_data_mutex); pthread_mutex_lock(&surface->mutex); if (surface->xdg_toplevel) @@ -141,12 +142,18 @@ void wayland_surface_destroy(struct wayland_surface *surface) if (surface->wl_surface) { + wl_surface_set_user_data(surface->wl_surface, NULL); wl_surface_destroy(surface->wl_surface); surface->wl_surface = NULL; } pthread_mutex_unlock(&surface->mutex); - pthread_mutex_unlock(&xdg_data_mutex); + pthread_mutex_unlock(&user_data_mutex); + + pthread_mutex_lock(&process_wayland.pointer.mutex); + if (process_wayland.pointer.surface == surface) + process_wayland.pointer.surface = NULL; + pthread_mutex_unlock(&process_wayland.pointer.mutex); if (surface->latest_window_buffer) wayland_shm_buffer_unref(surface->latest_window_buffer); diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 8932c2c9e8d..14d0c7e5575 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -67,7 +67,12 @@ struct wayland struct xdg_wm_base *xdg_wm_base; struct wl_shm *wl_shm; struct wl_seat *wl_seat; - struct wl_pointer *wl_pointer; + struct + { + struct wl_pointer *wl_pointer; + struct wayland_surface *surface; + pthread_mutex_t mutex; + } pointer; struct wl_list output_list; /* Protects the output_list and the wayland_output.current states. */ pthread_mutex_t output_mutex; @@ -150,6 +155,7 @@ void wayland_surface_clear_role(struct wayland_surface *surface) DECLSPEC_HIDDEN void wayland_surface_attach_shm(struct wayland_surface *surface, struct wayland_shm_buffer *shm_buffer, HRGN surface_damage_region) DECLSPEC_HIDDEN; +struct wayland_surface *wayland_surface_lock_proxy(struct wl_proxy *wl_proxy) DECLSPEC_HIDDEN; /********************************************************************** * Wayland SHM buffer -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> Also emit a synthetic motion event on pointer entry, to handle cases where the compositor doesn't send an initial motion event itself. --- dlls/winewayland.drv/Makefile.in | 2 +- dlls/winewayland.drv/wayland_pointer.c | 62 ++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/dlls/winewayland.drv/Makefile.in b/dlls/winewayland.drv/Makefile.in index 6bc03187d61..e1019ad8348 100644 --- a/dlls/winewayland.drv/Makefile.in +++ b/dlls/winewayland.drv/Makefile.in @@ -1,7 +1,7 @@ MODULE = winewayland.drv UNIXLIB = winewayland.so UNIX_CFLAGS = $(WAYLAND_CLIENT_CFLAGS) -UNIX_LIBS = -lwin32u $(WAYLAND_CLIENT_LIBS) $(PTHREAD_LIBS) +UNIX_LIBS = -lwin32u $(WAYLAND_CLIENT_LIBS) $(PTHREAD_LIBS) -lm SOURCES = \ display.c \ diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c index 1476c2369a9..29121d058cf 100644 --- a/dlls/winewayland.drv/wayland_pointer.c +++ b/dlls/winewayland.drv/wayland_pointer.c @@ -24,14 +24,71 @@ #include "config.h" +#include <math.h> + #include "waylanddrv.h" #include "wine/debug.h" WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv); +static struct wayland_surface *pointer_lock_surface(void) +{ + struct wayland_surface *surface; + + pthread_mutex_lock(&process_wayland.pointer.mutex); + + if ((surface = process_wayland.pointer.surface)) + { + pthread_mutex_lock(&surface->mutex); + /* Return the surface only if it is not under destruction. */ + if (surface->wl_surface) return surface; + pthread_mutex_unlock(&surface->mutex); + } + + pthread_mutex_unlock(&process_wayland.pointer.mutex); + + return NULL; +} + +static void pointer_unlock_surface(void) +{ + pthread_mutex_unlock(&process_wayland.pointer.surface->mutex); + pthread_mutex_unlock(&process_wayland.pointer.mutex); +} + static void pointer_handle_motion(void *data, struct wl_pointer *wl_pointer, uint32_t time, wl_fixed_t sx, wl_fixed_t sy) { + INPUT input = {0}; + RECT window_rect; + struct wayland_surface *surface; + int screen_x, screen_y; + + if (!(surface = pointer_lock_surface())) return; + + NtUserGetWindowRect(surface->hwnd, &window_rect); + + screen_x = round(wl_fixed_to_double(sx)) + window_rect.left; + screen_y = round(wl_fixed_to_double(sy)) + window_rect.top; + /* Sometimes, due to rounding, we may end up with pointer coordinates + * slightly outside the target window, so bring them within bounds. */ + if (screen_x >= window_rect.right) screen_x = window_rect.right - 1; + else if (screen_x < window_rect.left) screen_x = window_rect.left; + if (screen_y >= window_rect.bottom) screen_y = window_rect.bottom - 1; + else if (screen_y < window_rect.top) screen_y = window_rect.top; + + input.type = INPUT_MOUSE; + input.mi.dx = screen_x; + input.mi.dy = screen_y; + input.mi.dwFlags = MOUSEEVENTF_MOVE | MOUSEEVENTF_ABSOLUTE; + + TRACE("surface=%p hwnd=%p wayland_xy=%.2f,%.2f screen_xy=%d,%d\n", + surface, surface->hwnd, wl_fixed_to_double(sx), wl_fixed_to_double(sy), + screen_x, screen_y); + + __wine_send_input(surface->hwnd, &input, NULL); + + pointer_unlock_surface(); } static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, @@ -50,6 +107,11 @@ static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, pthread_mutex_unlock(&surface->mutex); } pthread_mutex_unlock(&process_wayland.pointer.mutex); + + /* Handle the enter as a motion, to account for cases where the + * window first appears beneath the pointer and won't get a separate + * motion event. */ + pointer_handle_motion(data, wl_pointer, 0, sx, sy); } static void pointer_handle_leave(void *data, struct wl_pointer *wl_pointer, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/winewayland.drv/wayland_pointer.c | 51 ++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c index 29121d058cf..6b4a5aacee9 100644 --- a/dlls/winewayland.drv/wayland_pointer.c +++ b/dlls/winewayland.drv/wayland_pointer.c @@ -24,6 +24,8 @@ #include "config.h" +#include <linux/input.h> +#undef SW_MAX /* Also defined in winuser.rh */ #include <math.h> #include "waylanddrv.h" @@ -138,6 +140,29 @@ static void pointer_handle_button(void *data, struct wl_pointer *wl_pointer, uint32_t serial, uint32_t time, uint32_t button, uint32_t state) { + INPUT input = {0}; + struct wayland_surface *surface; + + if (!(surface = pointer_lock_surface())) return; + + input.type = INPUT_MOUSE; + + switch (button) + { + case BTN_LEFT: input.mi.dwFlags = MOUSEEVENTF_LEFTDOWN; break; + case BTN_RIGHT: input.mi.dwFlags = MOUSEEVENTF_RIGHTDOWN; break; + case BTN_MIDDLE: input.mi.dwFlags = MOUSEEVENTF_MIDDLEDOWN; break; + default: break; + } + + if (state == WL_POINTER_BUTTON_STATE_RELEASED) input.mi.dwFlags <<= 1; + + TRACE("surface=%p hwnd=%p button=%#x state=%u\n", + surface, surface->hwnd, button, state); + + __wine_send_input(surface->hwnd, &input, NULL); + + pointer_unlock_surface(); } static void pointer_handle_axis(void *data, struct wl_pointer *wl_pointer, @@ -162,6 +187,32 @@ static void pointer_handle_axis_stop(void *data, struct wl_pointer *wl_pointer, static void pointer_handle_axis_discrete(void *data, struct wl_pointer *wl_pointer, uint32_t axis, int32_t discrete) { + INPUT input = {0}; + struct wayland_surface *surface; + + if (!(surface = pointer_lock_surface())) return; + + input.type = INPUT_MOUSE; + + switch (axis) + { + case WL_POINTER_AXIS_VERTICAL_SCROLL: + input.mi.dwFlags = MOUSEEVENTF_WHEEL; + input.mi.mouseData = -WHEEL_DELTA * discrete; + break; + case WL_POINTER_AXIS_HORIZONTAL_SCROLL: + input.mi.dwFlags = MOUSEEVENTF_HWHEEL; + input.mi.mouseData = WHEEL_DELTA * discrete; + break; + default: break; + } + + TRACE("surface=%p hwnd=%p axis=%u discrete=%d\n", + surface, surface->hwnd, axis, discrete); + + __wine_send_input(surface->hwnd, &input, NULL); + + pointer_unlock_surface(); } static const struct wl_pointer_listener pointer_listener = -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> Set the cursor image used for Wayland surfaces by using the Windows cursor bitmap data. --- dlls/winewayland.drv/wayland_pointer.c | 293 +++++++++++++++++++++++++ dlls/winewayland.drv/waylanddrv.h | 9 + dlls/winewayland.drv/waylanddrv_main.c | 1 + dlls/winewayland.drv/window.c | 18 ++ 4 files changed, 321 insertions(+) diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c index 6b4a5aacee9..31d8a1378be 100644 --- a/dlls/winewayland.drv/wayland_pointer.c +++ b/dlls/winewayland.drv/wayland_pointer.c @@ -27,6 +27,7 @@ #include <linux/input.h> #undef SW_MAX /* Also defined in winuser.rh */ #include <math.h> +#include <stdlib.h> #include "waylanddrv.h" #include "wine/debug.h" @@ -106,6 +107,14 @@ static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, { TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); process_wayland.pointer.surface = surface; + process_wayland.pointer.enter_serial = serial; + /* The cursor is undefined at every enter, so we set it again with + * the latest information we have. */ + wl_pointer_set_cursor(process_wayland.pointer.wl_pointer, + process_wayland.pointer.enter_serial, + surface->cursor.wl_surface, + surface->cursor.hotspot_x, + surface->cursor.hotspot_y); pthread_mutex_unlock(&surface->mutex); } pthread_mutex_unlock(&process_wayland.pointer.mutex); @@ -130,6 +139,7 @@ static void pointer_handle_leave(void *data, struct wl_pointer *wl_pointer, { TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); process_wayland.pointer.surface = NULL; + process_wayland.pointer.enter_serial = 0; } pthread_mutex_unlock(&surface->mutex); } @@ -245,3 +255,286 @@ void wayland_pointer_deinit(void) process_wayland.pointer.surface = NULL; pthread_mutex_unlock(&process_wayland.pointer.mutex); } + +/*********************************************************************** + * create_mono_cursor_buffer + * + * Create a wayland_shm_buffer for a monochrome cursor bitmap. + * + * Adapted from wineandroid.drv code. + */ +static struct wayland_shm_buffer *create_mono_cursor_buffer(HBITMAP bmp) +{ + struct wayland_shm_buffer *shm_buffer = NULL; + BITMAP bm; + char *mask = NULL; + unsigned int i, j, stride, mask_size, *ptr; + + if (!NtGdiExtGetObjectW(bmp, sizeof(bm), &bm)) goto done; + stride = ((bm.bmWidth + 15) >> 3) & ~1; + mask_size = stride * bm.bmHeight; + if (!(mask = malloc(mask_size))) goto done; + if (!NtGdiGetBitmapBits(bmp, mask_size, mask)) goto done; + + bm.bmHeight /= 2; + shm_buffer = wayland_shm_buffer_create(bm.bmWidth, bm.bmHeight, + WL_SHM_FORMAT_ARGB8888); + if (!shm_buffer) goto done; + + ptr = shm_buffer->map_data; + for (i = 0; i < bm.bmHeight; i++) + { + for (j = 0; j < bm.bmWidth; j++, ptr++) + { + int and = ((mask[i * stride + j / 8] << (j % 8)) & 0x80); + int xor = ((mask[(i + bm.bmHeight) * stride + j / 8] << (j % 8)) & 0x80); + if (!xor && and) + *ptr = 0; + else if (xor && !and) + *ptr = 0xffffffff; + else + /* we can't draw "invert" pixels, so render them as black instead */ + *ptr = 0xff000000; + } + } + +done: + free(mask); + return shm_buffer; +} + +/*********************************************************************** + * create_color_cursor_buffer + * + * Create a wayland_shm_buffer for a color cursor bitmap. + * + * Adapted from wineandroid.drv code. + */ +static struct wayland_shm_buffer *create_color_cursor_buffer(HDC hdc, HBITMAP color, + HBITMAP mask) +{ + struct wayland_shm_buffer *shm_buffer = NULL; + char buffer[FIELD_OFFSET(BITMAPINFO, bmiColors[256])]; + BITMAPINFO *info = (BITMAPINFO *)buffer; + BITMAP bm; + unsigned int *ptr, *bits = NULL; + unsigned char *mask_bits = NULL; + int i, j; + BOOL has_alpha = FALSE; + + if (!NtGdiExtGetObjectW(color, sizeof(bm), &bm)) goto failed; + + shm_buffer = wayland_shm_buffer_create(bm.bmWidth, bm.bmHeight, + WL_SHM_FORMAT_ARGB8888); + if (!shm_buffer) goto failed; + bits = shm_buffer->map_data; + + info->bmiHeader.biSize = sizeof(BITMAPINFOHEADER); + info->bmiHeader.biWidth = bm.bmWidth; + info->bmiHeader.biHeight = -bm.bmHeight; + info->bmiHeader.biPlanes = 1; + info->bmiHeader.biBitCount = 32; + info->bmiHeader.biCompression = BI_RGB; + info->bmiHeader.biSizeImage = bm.bmWidth * bm.bmHeight * 4; + info->bmiHeader.biXPelsPerMeter = 0; + info->bmiHeader.biYPelsPerMeter = 0; + info->bmiHeader.biClrUsed = 0; + info->bmiHeader.biClrImportant = 0; + + if (!NtGdiGetDIBitsInternal(hdc, color, 0, bm.bmHeight, bits, info, + DIB_RGB_COLORS, 0, 0)) + goto failed; + + for (i = 0; i < bm.bmWidth * bm.bmHeight; i++) + if ((has_alpha = (bits[i] & 0xff000000) != 0)) break; + + if (!has_alpha) + { + unsigned int width_bytes = (bm.bmWidth + 31) / 32 * 4; + /* generate alpha channel from the mask */ + info->bmiHeader.biBitCount = 1; + info->bmiHeader.biSizeImage = width_bytes * bm.bmHeight; + if (!(mask_bits = malloc(info->bmiHeader.biSizeImage))) goto failed; + if (!NtGdiGetDIBitsInternal(hdc, mask, 0, bm.bmHeight, mask_bits, + info, DIB_RGB_COLORS, 0, 0)) + goto failed; + ptr = bits; + for (i = 0; i < bm.bmHeight; i++) + { + for (j = 0; j < bm.bmWidth; j++, ptr++) + { + if (!((mask_bits[i * width_bytes + j / 8] << (j % 8)) & 0x80)) + *ptr |= 0xff000000; + } + } + free(mask_bits); + } + + /* Wayland requires pre-multiplied alpha values */ + for (ptr = bits, i = 0; i < bm.bmWidth * bm.bmHeight; ptr++, i++) + { + unsigned char alpha = *ptr >> 24; + if (alpha == 0) + { + *ptr = 0; + } + else if (alpha != 255) + { + *ptr = (alpha << 24) | + (((BYTE)(*ptr >> 16) * alpha / 255) << 16) | + (((BYTE)(*ptr >> 8) * alpha / 255) << 8) | + (((BYTE)*ptr * alpha / 255)); + } + } + + return shm_buffer; + +failed: + if (shm_buffer) wayland_shm_buffer_unref(shm_buffer); + free(mask_bits); + return NULL; +} + +/*********************************************************************** + * get_icon_info + * + * Local GetIconInfoExW helper implementation. + */ +static BOOL get_icon_info(HICON handle, ICONINFOEXW *ret) +{ + UNICODE_STRING module, res_name; + ICONINFO info; + + module.Buffer = ret->szModName; + module.MaximumLength = sizeof(ret->szModName) - sizeof(WCHAR); + res_name.Buffer = ret->szResName; + res_name.MaximumLength = sizeof(ret->szResName) - sizeof(WCHAR); + if (!NtUserGetIconInfo(handle, &info, &module, &res_name, NULL, 0)) return FALSE; + ret->fIcon = info.fIcon; + ret->xHotspot = info.xHotspot; + ret->yHotspot = info.yHotspot; + ret->hbmColor = info.hbmColor; + ret->hbmMask = info.hbmMask; + ret->wResID = res_name.Length ? 0 : LOWORD(res_name.Buffer); + ret->szModName[module.Length] = 0; + ret->szResName[res_name.Length] = 0; + return TRUE; +} + +static void wayland_surface_update_cursor(struct wayland_surface *surface, + HCURSOR hcursor) +{ + ICONINFOEXW info = {0}; + + if (!hcursor) goto clear_cursor; + + /* Create a new buffer for the specified cursor. */ + if (surface->cursor.shm_buffer) + { + wayland_shm_buffer_unref(surface->cursor.shm_buffer); + surface->cursor.shm_buffer = NULL; + } + + if (!get_icon_info(hcursor, &info)) + { + ERR("Failed to get icon info for cursor=%p\n", hcursor); + goto clear_cursor; + } + + if (info.hbmColor) + { + HDC hdc = NtGdiCreateCompatibleDC(0); + surface->cursor.shm_buffer = + create_color_cursor_buffer(hdc, info.hbmColor, info.hbmMask); + NtGdiDeleteObjectApp(hdc); + } + else + { + surface->cursor.shm_buffer = create_mono_cursor_buffer(info.hbmMask); + } + + if (info.hbmColor) NtGdiDeleteObjectApp(info.hbmColor); + if (info.hbmMask) NtGdiDeleteObjectApp(info.hbmMask); + + surface->cursor.hotspot_x = info.xHotspot; + surface->cursor.hotspot_y = info.yHotspot; + + if (!surface->cursor.shm_buffer) + { + ERR("Failed to create shm_buffer for cursor=%p\n", hcursor); + goto clear_cursor; + } + + /* Make sure the hotspot is valid. */ + if (surface->cursor.hotspot_x >= surface->cursor.shm_buffer->width || + surface->cursor.hotspot_y >= surface->cursor.shm_buffer->height) + { + surface->cursor.hotspot_x = surface->cursor.shm_buffer->width / 2; + surface->cursor.hotspot_y = surface->cursor.shm_buffer->height / 2; + } + + if (!surface->cursor.wl_surface) + { + surface->cursor.wl_surface = + wl_compositor_create_surface(process_wayland.wl_compositor); + if (!surface->cursor.wl_surface) + { + ERR("Failed to create wl_surface for cursor\n"); + goto clear_cursor; + } + } + + /* Commit the cursor buffer to the cursor surface. */ + wl_surface_attach(surface->cursor.wl_surface, + surface->cursor.shm_buffer->wl_buffer, 0, 0); + wl_surface_damage_buffer(surface->cursor.wl_surface, 0, 0, + surface->cursor.shm_buffer->width, + surface->cursor.shm_buffer->height); + wl_surface_commit(surface->cursor.wl_surface); + + return; + +clear_cursor: + if (surface->cursor.shm_buffer) + { + wayland_shm_buffer_unref(surface->cursor.shm_buffer); + surface->cursor.shm_buffer = NULL; + } + if (surface->cursor.wl_surface) + { + wl_surface_destroy(surface->cursor.wl_surface); + surface->cursor.wl_surface = NULL; + } +} + +/*********************************************************************** + * WAYLAND_SetCursor + */ +void WAYLAND_SetCursor(HWND hwnd, HCURSOR hcursor) +{ + struct wayland_surface *surface; + + TRACE("hwnd=%p hcursor=%p\n", hwnd, hcursor); + + pthread_mutex_lock(&process_wayland.pointer.mutex); + + if ((surface = wayland_surface_lock_hwnd(hwnd))) + { + wayland_surface_update_cursor(surface, hcursor); + + if (process_wayland.pointer.surface == surface) + { + wl_pointer_set_cursor(process_wayland.pointer.wl_pointer, + process_wayland.pointer.enter_serial, + surface->cursor.wl_surface, + surface->cursor.hotspot_x, + surface->cursor.hotspot_y); + } + + wl_display_flush(process_wayland.wl_display); + + pthread_mutex_unlock(&surface->mutex); + } + + pthread_mutex_unlock(&process_wayland.pointer.mutex); +} diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 14d0c7e5575..17025c37048 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -71,6 +71,7 @@ struct wayland { struct wl_pointer *wl_pointer; struct wayland_surface *surface; + uint32_t enter_serial; pthread_mutex_t mutex; } pointer; struct wl_list output_list; @@ -115,6 +116,12 @@ struct wayland_surface pthread_mutex_t mutex; uint32_t current_serial; struct wayland_shm_buffer *latest_window_buffer; + struct + { + struct wayland_shm_buffer *shm_buffer; + struct wl_surface *wl_surface; + int hotspot_x, hotspot_y; + } cursor; }; struct wayland_shm_buffer @@ -156,6 +163,7 @@ void wayland_surface_attach_shm(struct wayland_surface *surface, struct wayland_shm_buffer *shm_buffer, HRGN surface_damage_region) DECLSPEC_HIDDEN; struct wayland_surface *wayland_surface_lock_proxy(struct wl_proxy *wl_proxy) DECLSPEC_HIDDEN; +struct wayland_surface *wayland_surface_lock_hwnd(HWND hwnd) DECLSPEC_HIDDEN; /********************************************************************** * Wayland SHM buffer @@ -203,6 +211,7 @@ RGNDATA *get_region_data(HRGN region) DECLSPEC_HIDDEN; LRESULT WAYLAND_DesktopWindowProc(HWND hwnd, UINT msg, WPARAM wp, LPARAM lp) DECLSPEC_HIDDEN; void WAYLAND_DestroyWindow(HWND hwnd) DECLSPEC_HIDDEN; +void WAYLAND_SetCursor(HWND hwnd, HCURSOR hcursor) DECLSPEC_HIDDEN; BOOL WAYLAND_UpdateDisplayDevices(const struct gdi_device_manager *device_manager, BOOL force, void *param) DECLSPEC_HIDDEN; LRESULT WAYLAND_WindowMessage(HWND hwnd, UINT msg, WPARAM wp, LPARAM lp) DECLSPEC_HIDDEN; diff --git a/dlls/winewayland.drv/waylanddrv_main.c b/dlls/winewayland.drv/waylanddrv_main.c index 530cdd83d90..eb54efe1180 100644 --- a/dlls/winewayland.drv/waylanddrv_main.c +++ b/dlls/winewayland.drv/waylanddrv_main.c @@ -33,6 +33,7 @@ static const struct user_driver_funcs waylanddrv_funcs = { .pDesktopWindowProc = WAYLAND_DesktopWindowProc, .pDestroyWindow = WAYLAND_DestroyWindow, + .pSetCursor = WAYLAND_SetCursor, .pUpdateDisplayDevices = WAYLAND_UpdateDisplayDevices, .pWindowMessage = WAYLAND_WindowMessage, .pWindowPosChanged = WAYLAND_WindowPosChanged, diff --git a/dlls/winewayland.drv/window.c b/dlls/winewayland.drv/window.c index efe72baf92e..6f4867dd561 100644 --- a/dlls/winewayland.drv/window.c +++ b/dlls/winewayland.drv/window.c @@ -338,3 +338,21 @@ void wayland_window_flush(HWND hwnd) wayland_win_data_release(data); } + +/********************************************************************** + * wayland_surface_lock_hwnd + */ +struct wayland_surface *wayland_surface_lock_hwnd(HWND hwnd) +{ + struct wayland_win_data *data = wayland_win_data_get(hwnd); + struct wayland_surface *surface = NULL; + + if (data) + { + surface = data->wayland_surface; + if (surface) pthread_mutex_lock(&data->wayland_surface->mutex); + wayland_win_data_release(data); + } + + return surface; +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3686
As noted in the description there is the potential for bad interactions between the driver input event dispatching and the win32u window flushing mechanism. For an example of this just move the pointer around within the scrollbar in `notepad` (when using the `light` theme), which at least on my system cause artifacts. The root cause seems to be that the driver can queue enough (motion) messages to prevent the message loop from becoming idle (and thus flush the surfaces) for an extended period. If we reach the 50ms limit without having flushed, we are forced to flush in `dlls/win32u/dibdrv/dc.c:unlock_surface` at unfortunate points during rendering, leading to artifacts. For a more concrete description of what I understand to be happening using the scrollbar scenario (assume 3 Wayland motion events every 8ms, each scrollbar draw takes 20ms): ```plaintext Wayland event thread UI thread -------------------- --------- 0: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 0: Wayland: motion event 1 0: GetMessage -> WM_MOUSEMOVE 1 0: DrawScrollbar 8: Wayland: motion event 2 16: Wayland: motion event 3 20: GetMessage -> not idle, WM_MOUSEMOVE 2 20: DrawScrollbar 40: GetMessage -> not idle, WM_MOUSEMOVE 3 40: DrawScrollbar... 50: ...but we pass 50ms since last flush (at 0ms), so we flush before DrawScrollbar is done ``` Under X11, even though the motion event frequency is similar (again, let's say every 8ms), the X11 motion events are dispatched from the message loop thread one at a time, allowing the message loop to become idle, like so: ```plaintext UI thread --------- 0: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 0: X11: motion event 1 0: GetMessage -> WM_MOUSEMOVE 1 0: DrawScrollbar 20: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 20: X11: motion event 2 20: GetMessage -> WM_MOUSEMOVE 2 20: DrawScrollbar 40: GetMessage... no messages, idle so flush_window_surface(TRUE), wait ... ``` I've toyed with some ideas in the Wayland driver, but I haven't found a solution that's effective. I would appreciate any thoughts from someone more experienced in this area of `win32u`. Do you think that some kind of change in the `win32u` flushing logic could be an acceptable way forward? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43830
On Wed Aug 30 13:43:16 2023 +0000, Alexandros Frantzis wrote:
As noted in the description there is the potential for bad interactions between the driver input event dispatching and the win32u window flushing mechanism. For an example of this just move the pointer around within the scrollbar in `notepad` (when using the `light` theme), which at least on my system cause artifacts. The root cause seems to be that the driver can queue enough (motion) messages to prevent the message loop from becoming idle (and thus flush the surfaces) for an extended period. If we reach the 50ms limit without having flushed, we are forced to flush in `dlls/win32u/dibdrv/dc.c:unlock_surface` at unfortunate points during rendering, leading to artifacts. For a more concrete description of what I understand to be happening using the scrollbar scenario (assume 3 Wayland motion events every 8ms, each scrollbar draw takes 20ms): ```plaintext Wayland event thread UI thread -------------------- --------- 0: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 0: Wayland: motion event 1 0: GetMessage -> WM_MOUSEMOVE 1 0: DrawScrollbar 8: Wayland: motion event 2 16: Wayland: motion event 3 20: GetMessage -> not idle, WM_MOUSEMOVE 2 20: DrawScrollbar 40: GetMessage -> not idle, WM_MOUSEMOVE 3 40: DrawScrollbar... 50: ...but we pass 50ms since last flush (at 0ms), so we flush before DrawScrollbar is done ``` Under X11, even though the motion event frequency is similar (again, let's say every 8ms), the X11 motion events are dispatched from the message loop thread one at a time, allowing the message loop to become idle, like so: ```plaintext UI thread --------- 0: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 0: X11: motion event 1 0: GetMessage -> WM_MOUSEMOVE 1 0: DrawScrollbar 20: GetMessage... no messages, idle so flush_window_surface(TRUE), wait 20: X11: motion event 2 20: GetMessage -> WM_MOUSEMOVE 2 20: DrawScrollbar 40: GetMessage... no messages, idle so flush_window_surface(TRUE), wait ... ``` I've toyed with some ideas in the Wayland driver, but I haven't found a solution that's effective. I would appreciate any thoughts from someone more experienced in this area of `win32u`. Do you think that some kind of change in the `win32u` flushing logic could be an acceptable way forward? 20ms to draw a scrollbar feels a bit much, doesn't it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43849
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_surface.c:
WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv);
-/* Protects access to the user data of xdg_surface */ -static pthread_mutex_t xdg_data_mutex = PTHREAD_MUTEX_INITIALIZER; +/* Protects access to the wayland_surface user data of wayland objects */ +static pthread_mutex_t user_data_mutex = PTHREAD_MUTEX_INITIALIZER;
-static struct wayland_surface *wayland_surface_lock_xdg(struct xdg_surface *xdg_surface) +struct wayland_surface *wayland_surface_lock_proxy(struct wl_proxy *wl_proxy)
Please move the unrelated renames to a separate commit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43872
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/waylanddrv.h:
struct xdg_wm_base *xdg_wm_base; struct wl_shm *wl_shm; struct wl_seat *wl_seat; + struct + { struct wl_pointer *wl_pointer; + struct wayland_surface *surface; + pthread_mutex_t mutex;
Do we really need to keep the current surface around? Could the hwnd be enough? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43873
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_pointer.c:
uint32_t serial, struct wl_surface *wl_surface, wl_fixed_t sx, wl_fixed_t sy) { + struct wayland_surface *surface; + + if (!wl_surface) return; + + pthread_mutex_lock(&process_wayland.pointer.mutex); + if ((surface = wayland_surface_lock_proxy((struct wl_proxy *)wl_surface))) + { + TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); + process_wayland.pointer.surface = surface; + pthread_mutex_unlock(&surface->mutex); + } + pthread_mutex_unlock(&process_wayland.pointer.mutex);
I'm a bit uncomfortable with the double locking everywhere. I think it'd be less scary if we could do something like: ```C HWND hwnd = wayland_surface_get_hwnd(wl_surface); pthread_mutex_lock(&process_wayland.pointer.mutex); process_wayland.pointer.hwnd = hwnd; pthread_mutex_unlock(&process_wayland.pointer.mutex); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43874
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_pointer.c:
+ +static void pointer_unlock_surface(void) +{ + pthread_mutex_unlock(&process_wayland.pointer.surface->mutex); + pthread_mutex_unlock(&process_wayland.pointer.mutex); +} + static void pointer_handle_motion(void *data, struct wl_pointer *wl_pointer, uint32_t time, wl_fixed_t sx, wl_fixed_t sy) { + INPUT input = {0}; + RECT window_rect; + struct wayland_surface *surface; + int screen_x, screen_y; + + if (!(surface = pointer_lock_surface())) return; We get `enter, motion*, leave` sequences, can we instead assume that the hwnd under the cursor stays the same?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43875
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_pointer.c:
{ TRACE("surface=%p hwnd=%p\n", surface, surface->hwnd); process_wayland.pointer.surface = surface; + process_wayland.pointer.enter_serial = serial; + /* The cursor is undefined at every enter, so we set it again with + * the latest information we have. */ + wl_pointer_set_cursor(process_wayland.pointer.wl_pointer, + process_wayland.pointer.enter_serial, + surface->cursor.wl_surface, + surface->cursor.hotspot_x, + surface->cursor.hotspot_y);
Do we have to set the cursor here? Could it wait until `SetCursor` is called instead? Fwiw it should be called on every window/cursor change now, and I think it would save you the need to keep the window cursor surface around, unless it is required after `wl_pointer_set_cursor`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43876
On Wed Aug 30 13:43:16 2023 +0000, Rémi Bernon wrote:
20ms to draw a scrollbar feels a bit much, doesn't it? Indeed it does, but this is an actual value I have seen on my system (with the `light` theme). It's not always that bad, although a quick test shows that the mean duration of `UXTHEME_ScrollBarDraw` is ~13ms with Wayland and ~10ms with X11, which is still a lot. FWIW, the majority of the time is spent drawing the top and bottom arrows of the scrollbar.
There is certainly room for optimization here, but the timing of this particular scenario is just an example. Any frequency of motion events/messages that's higher than the message loop frequency will cause the loop to not become idle, and will lead to artifacts if sustained long enough (e.g., consider motion events every 2ms with a 3ms draw/loop period). I suspect we will have a similar problem with sustained resize events. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43950
On Wed Aug 30 15:38:10 2023 +0000, Rémi Bernon wrote:
Please move the unrelated renames to a separate commit. Ack.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43951
On Wed Aug 30 15:38:11 2023 +0000, Rémi Bernon wrote:
Do we really need to keep the current surface around? Could the hwnd be enough? Please see other related comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43952
On Wed Aug 30 15:38:12 2023 +0000, Rémi Bernon wrote:
I'm a bit uncomfortable with the double locking everywhere. I think it'd be less scary if we could do something like: ```C HWND hwnd = wayland_surface_get_hwnd(wl_surface); pthread_mutex_lock(&process_wayland.pointer.mutex); process_wayland.pointer.hwnd = hwnd; pthread_mutex_unlock(&process_wayland.pointer.mutex); ``` My concern with the above is that the window (and backing wayland surface) could be destroyed (from a different thread) between getting the `hwnd` and setting it as the pointer focus, so we will end up with an invalid `pointer.hwnd` value. Although I am not a big fan of double locking either, having an inconsistent state like this also scares me.
That being said, I think that *at the moment* such an inconsistency would be inconsequential, *as long as the HWND value is not reused* (at least not in any reasonable timeframe). Is there a strong practical guarantee about this? In such a case sending input events to the destroyed HWND would be nops and we would use `wayland_surface_lock_hwnd()` to get the surface when we need it (e.g, in `enter` to set the cursor, and in the future we will need it in `motion` for proper input coordinate system transformations), which would return NULL for a destroyed HWND. If this approach sounds more appealing than what is currently proposed, I will need to explore it more and ensure (for some value of "ensure") that there aren't any other races or edge cases that could affect us (especially related to concurrent window destruction). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43953
On Wed Aug 30 15:38:13 2023 +0000, Rémi Bernon wrote:
We get `enter, motion*, leave` sequences, can we instead assume that the hwnd under the cursor stays the same? I am not sure I understand the question. Is this about tracking the focused `HWND` instead of the focused surface (i.e., what's proposed in the other comments), or is this about something different?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43954
On Wed Aug 30 15:38:14 2023 +0000, Rémi Bernon wrote:
Do we have to set the cursor here? Could it wait until `SetCursor` is called instead? Fwiw it should be called on every window/cursor change now, and I think it would save you the need to keep the window cursor surface around, unless it is required after `wl_pointer_set_cursor`. We need to keep the cursor `wl_surface` around, since destroying a `wl_surface` while it has a role (the cursor role in this case) is a protocol error.
Concerning whether we have to set the cursor here or wait for `SetCursor`: what I am seeing is that after leaving a window for the desktop area and then reentering that window we don't get a new `SetCursor` callback. That's true for both the Wayland driver and the X11 driver. If this is not intended, what I assume is happening is that since neither driver is informing Wine about leave events, from Wine's perspective the cursor stays within that window, although it has left the window in the native system. This is less of a problem for WineX11 because `XDefineCursor` is persistent, whereas `wl_pointer_set_cursor` is ephemeral (valid only while the surface has the focus) and needs to be called after every `enter` event. The proposed approach effectively matches the WineX11 behavior. If we could guarantee that after *every* `enter` we would get a `SetCursor`, we could drop the `wl_pointer_set_cursor` here. Even then it's not clear that doing so would provide the best user experience. Different compositors deal differently with the lack of a set cursor after `enter`: some show no cursor, some keep whatever cursor was shown before entering. If a compositor does the former, then there could be a short cursor flicker when entering the surface: after `enter` no cursor is set, so the compositor hides it, after a bit `SetCursor` is called and the right cursor is set. If the compositor does the latter then the transition will be smoother. Of course, even with the current approach things are not perfect (but at least match WineX11), since on enter we will (possibly transiently) use whatever cursor was last used for that window, which could be incorrect. In the end, if the transitions are quick enough it may not matter what approach we use. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43955
what I am seeing is that after leaving a window for the desktop area and then reentering that window we don't get a new SetCursor callback. That's true for both the Wayland driver and the X11 driver.
A clarification: this is true only if the cursor for the window doesn't change on reentry -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_43965
On Fri Sep 1 06:44:09 2023 +0000, Alexandros Frantzis wrote:
I am not sure I understand the question. Is this about tracking the focused `HWND` instead of the focused surface (i.e., what's proposed in the other comments), or is this about something different? Yes it's pretty much the same comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44039
On Thu Aug 31 12:57:08 2023 +0000, Alexandros Frantzis wrote:
what I am seeing is that after leaving a window for the desktop area and then reentering that window we don't get a new SetCursor callback. That's true for both the Wayland driver and the X11 driver. A clarification: this is true only if the cursor for the window doesn't change on reentry I think we could have a notification on re-entry, although it's not critical. I can see why it doesn't, as we only monitor Wine windows.
Anyway, if you have to keep the cursor surface around then it's pointless. I was mostly interested in getting rid of the cursor surface on every window surface, I find the `surface->cursor.wl_surface` a bit confusing, too many "surface"s around. It's not a big deal though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44040
On Thu Aug 31 12:01:48 2023 +0000, Alexandros Frantzis wrote:
My concern with the above is that the window (and backing wayland surface) could be destroyed (from a different thread) between getting the `hwnd` and setting it as the pointer focus, so we will end up with an invalid `pointer.hwnd` value. Although I am not a big fan of double locking either, having an inconsistent state like this also scares me. That being said, I think that *at the moment* such an inconsistency would be inconsequential, *as long as the HWND value is not reused* (at least not in any reasonable timeframe). Is there a strong practical guarantee about this? In such a case sending input events to the destroyed HWND would be nops and we would use `wayland_surface_lock_hwnd()` to get the surface when we need it (e.g, in `enter` to set the cursor, and in the future we will need it in `motion` for proper input coordinate system transformations), which would return NULL for a destroyed HWND. If this approach sounds more appealing than what is currently proposed, I will need to explore it more and ensure (for some value of "ensure") that there aren't any other races or edge cases that could affect us (especially related to concurrent window destruction). I believe user handles have a generation that should guarantee against re-use.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44041
On Fri Sep 1 06:43:43 2023 +0000, Rémi Bernon wrote:
I believe user handles have a generation that should guarantee against re-use. I haven't been following these patches, so this may just be noise, but if we're the user driver and we get notified when windows are destroyed anyway, why is this a problem?
Actually, why can't we just look up the window inside the mutex? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44065
On Fri Sep 1 16:57:42 2023 +0000, Zebediah Figura wrote:
I haven't been following these patches, so this may just be noise, but if we're the user driver and we get notified when windows are destroyed anyway, why is this a problem? Actually, why can't we just look up the window inside the mutex? The pointer events are being handled in a separate thread from the windows owner threads.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44070
I believe user handles have a generation that should guarantee against re-use.
Thanks for the pointer. I looked at the handle code and the generation takes up two bytes, which means that it will be recycled pretty soon (a freed handle index is immediately available for reuse with a new generation). In fact, I wrote a win32 program that ends up recycling a HWND value in less than 2 seconds under Wine, so I wouldn't want to rely on HWNDs being unique. In practice it would require either a misbehaving or malicious program and unfortunate timings to get into such a state in a way that affects the driver, but this doesn't really alleviate my concerns. In any case, HWND recycling is only one of the concerns. The requirements for the alternatives are: 1. No double locking (between `pointer.mutex` and others) 2. Handle concurrent window destruction 3. Handle concurrent SetCursor 4. Take into account that we will likely need access to `wayland_surface` from within many of the pointer handlers anyway 5. Guard against HWND recycling (related to (2)) My current WIP best effort is at: https://gitlab.winehq.org/afrantzis/wine/-/commits/wayland-part-6-no-double-... (all changes are in last commit). Please let me know what you think. My overall impression of trying to think through the various alternatives, is that avoiding double locking involves a lot of additional multi-threading synchronization complexity and assumptions. The complexity is high enough that although I tried hard to verify that the code is correct, I still don't feel 100% confident that I haven't missed some edge case, and that's not a good sign. The high complexity concern stands even if we decide to not care about (5) and just go for the rest (see for example the code in `pointer_handle_enter` in the `wayland-part-6-no-double-lock` branch). It could be that I am missing something obvious, or a shift in perspective, or some additional assumptions, that would help produce a simpler and better alternative. Perhaps people like the current alternative branch better than the double locking anyway. However, at this time, I still find that the double locking approach is simpler to reason about than anything else I have come up with that tackles all of (2)-(5). Also, its failure mode is well known (i.e., deadlock) and "safe", compared to the uncharted territory of even more intricate thread synchronization, the possibility for inconsistent state and risk of user input reaching the wrong window. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44196
Take into account that we will likely need access to `wayland_surface` from within many of the pointer handlers anyway
What will we need it for exactly?
In fact, I wrote a win32 program that ends up recycling a HWND value in less than 2 seconds under Wine, so I wouldn't want to rely on HWNDs being unique. In practice it would require either a misbehaving or malicious program and unfortunate timings to get into such a state in a way that affects the driver, but this doesn't really alleviate my concerns.
My impression is that you're trying to make it unnecessarily robust. Of course, correctness is important, but I don't think we really care about malicious or misbehaving programs in general. I may be wrong but my gut feeling with mouse input is that it is updated often enough for such transitional state issues to not really be a problem in practice. If a window is destroyed / created, you will get a lot of different and concurrent mouse events anyway, and I'm pretty sure there's plenty of other places in the input stack where the target window validity is checked already, or where stale hwnd values are used.
The high complexity concern stands even if we decide to not care about (5) and just go for the rest (see for example the code in `pointer_handle_enter` in the `wayland-part-6-no-double-lock` branch).
What about keeping a single "last used" surface for the cursor and use it on enter event, until SetCursor is eventually called if it needs to be updated? You also wouldn't need the current surface for that. -- Also, this is only some suggestions to make things simpler. If you think it's better to be more robust here and you prefer to keep the locks, I can probably live with it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44201
Take into account that we will likely need access to wayland_surface from within many of the pointer handlers anyway What will we need it for exactly?
We will need per-surface info to be able to properly translate between Wayland per-surface coordinates and wine coordinates, when we introduce support for surface scaling (needed for hidpi and also mode change emulation). This is mainly needed for pointer motion (and in the future, relative motion) events. The goal of this item was to ensure that I had at least thought about this and didn't end up with a design that made it difficult to access such information (`wayland_surface_lock_hwnd()` should be fine for this, especially given the relaxed robustness requirements, see below).
My impression is that you're trying to make it unnecessarily robust.
I think this is indeed the core difference of the approaches we have been discussing. My goal has been maximizing robustness because 1. this would be my default approach lacking additional information and 2. I was under the impression that it would be a requirement for the driver. If robustness (against the kind of edge cases discussed in this thread) is not a strict requirement, then that provides me with the necessary perspective shift to make me more comfortable selecting a simpler solution. In such a case, I just want to make sure that the trade-offs have been properly explored and understood both by me and the reviewer(s).
What about keeping a single "last used" surface for the cursor and use it on enter event,
Yes, this will help simplify the code, thanks. This approach could (statistically) lead to transitionally using the wrong cursor on enter a bit more often compared to maintaining a per-surface cursor, but I think that's an acceptable compromise. In light of all the above I have prototyped a much simpler solution (with relaxed robustness guarantees) here: https://gitlab.winehq.org/afrantzis/wine/-/commits/wayland-part-6-no-double-... (changes in the last commit) If given all the discussion in this thread you find such a solution acceptable, then I would be fine going forward with it. And I guess we can always increase robustness (or some particular aspects of it, as needed) in the future if we have any issues. Thanks! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44294
On Tue Sep 5 11:38:19 2023 +0000, Alexandros Frantzis wrote:
Take into account that we will likely need access to wayland_surface from within many of the pointer handlers anyway What will we need it for exactly? We will need per-surface info to be able to properly translate between Wayland per-surface coordinates and wine coordinates, when we introduce support for surface scaling (needed for hidpi and also mode change emulation). This is mainly needed for pointer motion (and in the future, relative motion) events. The goal of this item was to ensure that I had at least thought about this and didn't end up with a design that made it difficult to access such information (`wayland_surface_lock_hwnd()` should be fine for this, especially given the relaxed robustness requirements, see below). My impression is that you're trying to make it unnecessarily robust. I think this is indeed the core difference of the approaches we have been discussing. My goal has been maximizing robustness because 1. this would be my default approach lacking additional information and 2. I was under the impression that it would be a requirement for the driver. If robustness (against the kind of edge cases discussed in this thread) is not a strict requirement, then that provides me with the necessary perspective shift to make me more comfortable selecting a simpler solution. In such a case, I just want to make sure that the trade-offs have been properly explored and understood both by me and the reviewer(s). What about keeping a single "last used" surface for the cursor and use it on enter event, Yes, this will help simplify the code, thanks. This approach could (statistically) lead to transitionally using the wrong cursor on enter a bit more often compared to maintaining a per-surface cursor, but I think that's an acceptable compromise. In light of all the above I have prototyped a much simpler solution (with relaxed robustness guarantees) here: https://gitlab.winehq.org/afrantzis/wine/-/commits/wayland-part-6-no-double-... (changes in the last commit) If given all the discussion in this thread you find such a solution acceptable, then I would be fine going forward with it. And I guess we can always increase robustness (or some particular aspects of it, as needed) in the future if we have any issues. Thanks! @julliard Could you confirm whether you expect the drivers to be strictly robust against these kind of edge cases (input received on invalid windows, during their destruction), or if it is alright to be more lax and rely on `hwnd` being validated at some point, assuming it doesn't cause any problems in the general case?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44301
Could you confirm whether you expect the drivers to be strictly robust against these kind of edge cases (input received on invalid windows, during their destruction), or if it is alright to be more lax and rely on `hwnd` being validated at some point, assuming it doesn't cause any problems in the general case?
In general, the right way is to handle things that affect a window in the thread that owns the window, then no locking is necessary. That happens automatically for anything triggered by a window message. For other things, usually we send an internal message to switch processing to the right thread. If you are accessing the window from a different thread, then of course proper locking is needed. If you have to do that a lot, it's probably a sign that the design is wrong though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44311
In general, the right way is to handle things that affect a window in the thread that owns the window, then no locking is necessary. That happens automatically for anything triggered by a window message. For other things, usually we send an internal message to switch processing to the right thread.
In previous MR discussions I touched on some aspects of why the driver is using the current design (separate dispatch thread), but I would like to use this opportunity to present some additional justification. In contrast with, e.g., X11, mouse events in Wayland are emitted as part of the `wl_pointer` global object, rather than any `wl_surface` object. Since the association of `wl_pointer` with any `wl_surface`/`HWND` is ephemeral, we can't assign the dispatch of `wl_pointer` events to any particular window thread beforehand. Also due to how dispatching works we would not able to safely change the target event queue dynamically in this case (e.g., imagine if on each `enter` we could specify a new thread event queue for further input events to be queued to). The same stands for `wl_keyboard` objects (coming in the future). For all its drawbacks (i.e., not ideal in terms of Wine integration, and requiring additional thread synchronization) I have found that a separate Wayland dispatch thread is much simpler to reason about and deal with compared to other alternatives I have experimented with. And, as you mention, if we really need something to happen in the context of the window thread message loop, we have the escape hatch of a using an internal window message. Note, however, that using an internal message wouldn't help with our current concern, since it wouldn't mitigate the problem of ensuring that any stored HWND remains valid and has not been recycled during the event dispatch.
If you are accessing the window from a different thread, then of course proper locking is needed. If you have to do that a lot, it's probably a sign that the design is wrong though.
Do I understand correctly then, that robustness against HWND invalidation/recycling, in the context that @rbernon mentioned, is considered a requirement for the driver? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44383
Do I understand correctly then, that robustness against HWND invalidation/recycling, in the context that @rbernon mentioned, is considered a requirement for the driver?
Recycling can be ignored, but we certainly can't have the driver crash because a window was destroyed. The usual way to handle that is to refcount the structure associated with the window. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44415
Recycling can be ignored, but we certainly can't have the driver crash because a window was destroyed. The usual way to handle that is to refcount the structure associated with the window.
Thanks for the clarification. Driver crashes are certainly out of the question in all circumstances; accesses to internal structures are properly protected in all of the proposed alternatives. The worst case in the simpler version is that we send an input message to a recently destroyed HWND (which I expect should be a nop), or the wrong window in the case of HWND recycling (the edge case which I was more concerned about, but seems we can now ignore). @rbernon If I am not misunderstanding the conclusion of this discussion, it would be fine to go with an approach along the lines of the simpler solution (https://gitlab.winehq.org/afrantzis/wine/-/commits/wayland-part-6-no-double-...). If this also matches your understanding, please let me know and I will update the MR accordingly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44416
On Wed Sep 6 14:13:12 2023 +0000, Alexandros Frantzis wrote:
Recycling can be ignored, but we certainly can't have the driver crash because a window was destroyed. The usual way to handle that is to refcount the structure associated with the window. Thanks for the clarification. Driver crashes are certainly out of the question in all circumstances; accesses to internal structures are properly protected in all of the proposed alternatives. The worst case in the simpler version is that we send an input message to a recently destroyed HWND (which I expect should be a nop), or the wrong window in the case of HWND recycling (the edge case which I was more concerned about, but seems we can now ignore). @rbernon If I am not misunderstanding the conclusion of this discussion, it would be fine to go with an approach along the lines of the simpler solution (https://gitlab.winehq.org/afrantzis/wine/-/commits/wayland-part-6-no-double-...). If this also matches your understanding, please let me know and I will update the MR accordingly. This is also my understanding. Fwiw I believe macdrv also behaves similarly. I don't know if macdrv should be trusted as a reference of a good driver code, but at least there's a precedent.
It delegates the input message processing to the window thread, holding a reference to the host window in the event, and retrieving its hwnd from the macdrv event handler (this is different from what you are going to do, calling __wine_send_input directly). *But* the input event is received on a different thread, and I think it's possible for the Win32 window to be destroyed before the input event is processed, thus ending up using a stale hwnd. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44421
On Wed Sep 6 15:07:32 2023 +0000, Rémi Bernon wrote:
This is also my understanding. Fwiw I believe macdrv also behaves similarly. I don't know if macdrv should be trusted as a reference of a good driver code, but at least there's a precedent. It delegates the input message processing to the window thread, holding a reference to the host window in the event, and retrieving its hwnd from the macdrv event handler (this is different from what you are going to do, calling __wine_send_input directly). *But* the input event is received on a different thread, and I think it's possible for the Win32 window to be destroyed before the input event is processed, thus ending up using a stale hwnd. Fwiw you could perhaps also do wl_surface_set_user_data(wl_surface, 0) when it is destroyed, but I don't know how whether it synchronizes properly or if it would then require locking.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3686#note_44422
participants (5)
-
Alexandre Julliard (＠julliard) -
Alexandros Frantzis
-
Alexandros Frantzis (＠afrantzis)
-
Rémi Bernon
-
Zebediah Figura (＠zfigura)