This MR optimizes and makes correctness improvements to the window_surface flush implementation:
1. Implement a buffer queue to avoid constantly allocating new buffers. 2. Take into account the bounds of the window_surface flush and track per-buffer damage to reduce copying from the window_surface. 3. Copy from the window_surface only pixel data that lies within the flushed bounds. Maintain the latest contents for pixels outside these bounds, by copying them from the latest valid buffer. 4. Communicate damaged surface regions to the compositor to allow it to perform more efficient pixel data transfers (e.g., texture uploads).
Thanks!
-- v3: winewayland.drv: Send surface damage region to the compositor. winewayland.drv: Update only the flushed bounds from the window_surface. winewayland.drv: Track damaged buffer regions. winewayland.drv: Use a buffer queue for window_surface.
From: Alexandros Frantzis alexandros.frantzis@collabora.com
Create a buffer queue to hold the SHM buffers for each window_surface, to be able to reuse released buffers instead of constantly creating new ones. --- dlls/winewayland.drv/wayland_surface.c | 29 ++++- dlls/winewayland.drv/waylanddrv.h | 6 +- dlls/winewayland.drv/window_surface.c | 174 +++++++++++++++++++++++-- 3 files changed, 192 insertions(+), 17 deletions(-)
diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index ad30745fbc5..43ba853c2c5 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -221,6 +221,9 @@ void wayland_surface_clear_role(struct wayland_surface *surface) * wayland_surface_attach_shm * * Attaches a SHM buffer to a wayland surface. + * + * The buffer is marked as unavailable until committed and subsequently + * released by the compositor. */ void wayland_surface_attach_shm(struct wayland_surface *surface, struct wayland_shm_buffer *shm_buffer) @@ -228,19 +231,34 @@ void wayland_surface_attach_shm(struct wayland_surface *surface, TRACE("surface=%p shm_buffer=%p (%dx%d)\n", surface, shm_buffer, shm_buffer->width, shm_buffer->height);
+ shm_buffer->busy = TRUE; + wayland_shm_buffer_ref(shm_buffer); + wl_surface_attach(surface->wl_surface, shm_buffer->wl_buffer, 0, 0); wl_surface_damage_buffer(surface->wl_surface, 0, 0, shm_buffer->width, shm_buffer->height); }
/********************************************************************** - * wayland_shm_buffer_destroy + * wayland_shm_buffer_ref * - * Destroys a SHM buffer. + * Increases the reference count of a SHM buffer. */ -void wayland_shm_buffer_destroy(struct wayland_shm_buffer *shm_buffer) +void wayland_shm_buffer_ref(struct wayland_shm_buffer *shm_buffer) { - TRACE("%p map=%p\n", shm_buffer, shm_buffer->map_data); + InterlockedIncrement(&shm_buffer->ref); +} + +/********************************************************************** + * wayland_shm_buffer_unref + * + * Decreases the reference count of a SHM buffer (and may destroy it). + */ +void wayland_shm_buffer_unref(struct wayland_shm_buffer *shm_buffer) +{ + if (InterlockedDecrement(&shm_buffer->ref) > 0) return; + + TRACE("destroying %p map=%p\n", shm_buffer, shm_buffer->map_data);
if (shm_buffer->wl_buffer) wl_buffer_destroy(shm_buffer->wl_buffer); @@ -284,6 +302,7 @@ struct wayland_shm_buffer *wayland_shm_buffer_create(int width, int height,
TRACE("%p %dx%d format=%d size=%d\n", shm_buffer, width, height, format, size);
+ shm_buffer->ref = 1; shm_buffer->width = width; shm_buffer->height = height; shm_buffer->map_size = size; @@ -340,6 +359,6 @@ struct wayland_shm_buffer *wayland_shm_buffer_create(int width, int height, err: if (fd >= 0) close(fd); if (handle) NtClose(handle); - if (shm_buffer) wayland_shm_buffer_destroy(shm_buffer); + if (shm_buffer) wayland_shm_buffer_unref(shm_buffer); return NULL; } diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 0afc03f4147..5abc5ae2d43 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -107,10 +107,13 @@ struct wayland_surface
struct wayland_shm_buffer { + struct wl_list link; struct wl_buffer *wl_buffer; int width, height; void *map_data; size_t map_size; + BOOL busy; + LONG ref; };
/********************************************************************** @@ -145,7 +148,8 @@ void wayland_surface_attach_shm(struct wayland_surface *surface,
struct wayland_shm_buffer *wayland_shm_buffer_create(int width, int height, enum wl_shm_format format) DECLSPEC_HIDDEN; -void wayland_shm_buffer_destroy(struct wayland_shm_buffer *shm_buffer) DECLSPEC_HIDDEN; +void wayland_shm_buffer_ref(struct wayland_shm_buffer *shm_buffer) DECLSPEC_HIDDEN; +void wayland_shm_buffer_unref(struct wayland_shm_buffer *shm_buffer) DECLSPEC_HIDDEN;
/********************************************************************** * Wayland window surface diff --git a/dlls/winewayland.drv/window_surface.c b/dlls/winewayland.drv/window_surface.c index ca613c667c6..40129a8977a 100644 --- a/dlls/winewayland.drv/window_surface.c +++ b/dlls/winewayland.drv/window_surface.c @@ -32,11 +32,20 @@
WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv);
+struct wayland_buffer_queue +{ + struct wl_event_queue *wl_event_queue; + struct wl_list buffer_list; + int width; + int height; +}; + struct wayland_window_surface { struct window_surface header; HWND hwnd; struct wayland_surface *wayland_surface; + struct wayland_buffer_queue *wayland_buffer_queue; RECT bounds; void *bits; pthread_mutex_t mutex; @@ -59,11 +68,143 @@ static void buffer_release(void *data, struct wl_buffer *buffer) { struct wayland_shm_buffer *shm_buffer = data; TRACE("shm_buffer=%p\n", shm_buffer); - wayland_shm_buffer_destroy(shm_buffer); + shm_buffer->busy = FALSE; + wayland_shm_buffer_unref(shm_buffer); }
static const struct wl_buffer_listener buffer_listener = { buffer_release };
+/********************************************************************** + * wayland_buffer_queue_destroy + * + * Destroys a buffer queue and any contained buffers. + */ +static void wayland_buffer_queue_destroy(struct wayland_buffer_queue *queue) +{ + struct wayland_shm_buffer *shm_buffer, *next; + + wl_list_for_each_safe(shm_buffer, next, &queue->buffer_list, link) + { + wl_list_remove(&shm_buffer->link); + wl_list_init(&shm_buffer->link); + /* Since this buffer may still be busy, attach it to the per-process + * wl_event_queue to handle any future buffer release events. */ + wl_proxy_set_queue((struct wl_proxy *)shm_buffer->wl_buffer, + process_wayland.wl_event_queue); + wayland_shm_buffer_unref(shm_buffer); + } + + if (queue->wl_event_queue) + { + /* Dispatch the event queue before destruction to process any + * pending buffer release events. This is required after changing + * the buffer proxy event queue in the previous step, to avoid + * missing any events. */ + wl_display_dispatch_queue_pending(process_wayland.wl_display, + queue->wl_event_queue); + wl_event_queue_destroy(queue->wl_event_queue); + } + + free(queue); +} + +/********************************************************************** + * wayland_buffer_queue_create + * + * Creates a buffer queue containing buffers with the specified width and height. + */ +static struct wayland_buffer_queue *wayland_buffer_queue_create(int width, int height) +{ + struct wayland_buffer_queue *queue; + + queue = calloc(1, sizeof(*queue)); + if (!queue) goto err; + + queue->wl_event_queue = wl_display_create_queue(process_wayland.wl_display); + if (!queue->wl_event_queue) goto err; + queue->width = width; + queue->height = height; + + wl_list_init(&queue->buffer_list); + + return queue; + +err: + if (queue) wayland_buffer_queue_destroy(queue); + return NULL; +} + +/********************************************************************** + * wayland_buffer_queue_get_free_buffer + * + * Gets a free buffer from the buffer queue. If no free buffers + * are available this function blocks until it can provide one. + */ +static struct wayland_shm_buffer *wayland_buffer_queue_get_free_buffer(struct wayland_buffer_queue *queue) +{ + struct wayland_shm_buffer *shm_buffer; + + TRACE("queue=%p\n", queue); + + while (TRUE) + { + int nbuffers = 0; + + /* Dispatch any pending buffer release events. */ + wl_display_dispatch_queue_pending(process_wayland.wl_display, + queue->wl_event_queue); + + /* Search through our buffers to find an available one. */ + wl_list_for_each(shm_buffer, &queue->buffer_list, link) + { + if (!shm_buffer->busy) goto out; + nbuffers++; + } + + /* Dynamically create up to 3 buffers. */ + if (nbuffers < 3) + { + shm_buffer = wayland_shm_buffer_create(queue->width, queue->height, + WL_SHM_FORMAT_XRGB8888); + if (shm_buffer) + { + /* Buffer events go to their own queue so that we can dispatch + * them independently. */ + wl_proxy_set_queue((struct wl_proxy *) shm_buffer->wl_buffer, + queue->wl_event_queue); + wl_buffer_add_listener(shm_buffer->wl_buffer, &buffer_listener, + shm_buffer); + wl_list_insert(&queue->buffer_list, &shm_buffer->link); + goto out; + } + else if (nbuffers < 2) + { + /* If we failed to allocate a new buffer, but we have at least two + * buffers busy, there is a good chance the compositor will + * eventually release one of them, so dispatch events and wait + * below. Otherwise, give up and return a NULL buffer. */ + ERR(" => failed to acquire buffer\n"); + return NULL; + } + } + + /* We don't have any buffers available, so block waiting for a buffer + * release event. */ + if (wl_display_dispatch_queue(process_wayland.wl_display, + queue->wl_event_queue) == -1) + { + return NULL; + } + } + +out: + TRACE(" => %p %dx%d map=[%p, %p)\n", + shm_buffer, shm_buffer->width, shm_buffer->height, shm_buffer->map_data, + (unsigned char*)shm_buffer->map_data + shm_buffer->map_size); + + return shm_buffer; +} + /*********************************************************************** * wayland_window_surface_lock */ @@ -119,33 +260,30 @@ static void wayland_window_surface_set_region(struct window_surface *window_surf static void wayland_window_surface_flush(struct window_surface *window_surface) { struct wayland_window_surface *wws = wayland_window_surface_cast(window_surface); - struct wayland_shm_buffer *shm_buffer; + struct wayland_shm_buffer *shm_buffer = NULL; BOOL flushed = FALSE;
wayland_window_surface_lock(window_surface);
if (IsRectEmpty(&wws->bounds)) goto done;
- if (!wws->wayland_surface) + if (!wws->wayland_surface || !wws->wayland_buffer_queue) { - ERR("missing wayland surface, returning\n"); + ERR("missing wayland surface=%p or buffer_queue=%p, returning\n", + wws->wayland_surface, wws->wayland_buffer_queue); goto done; }
TRACE("surface=%p hwnd=%p surface_rect=%s bounds=%s\n", wws, wws->hwnd, wine_dbgstr_rect(&wws->header.rect), wine_dbgstr_rect(&wws->bounds));
- shm_buffer = wayland_shm_buffer_create(wws->info.bmiHeader.biWidth, - abs(wws->info.bmiHeader.biHeight), - WL_SHM_FORMAT_XRGB8888); + shm_buffer = wayland_buffer_queue_get_free_buffer(wws->wayland_buffer_queue); if (!shm_buffer) { - ERR("failed to create Wayland SHM buffer, returning\n"); + ERR("failed to acquire Wayland SHM buffer, returning\n"); goto done; }
- wl_buffer_add_listener(shm_buffer->wl_buffer, &buffer_listener, shm_buffer); - memcpy(shm_buffer->map_data, wws->bits, shm_buffer->map_size);
pthread_mutex_lock(&wws->wayland_surface->mutex); @@ -158,7 +296,6 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) else { TRACE("Wayland surface not configured yet, not flushing\n"); - wayland_shm_buffer_destroy(shm_buffer); } pthread_mutex_unlock(&wws->wayland_surface->mutex); wl_display_flush(process_wayland.wl_display); @@ -178,6 +315,8 @@ static void wayland_window_surface_destroy(struct window_surface *window_surface TRACE("surface=%p\n", wws);
pthread_mutex_destroy(&wws->mutex); + if (wws->wayland_buffer_queue) + wayland_buffer_queue_destroy(wws->wayland_buffer_queue); free(wws->bits); free(wws); } @@ -254,5 +393,18 @@ void wayland_window_surface_update_wayland_surface(struct window_surface *window
wws->wayland_surface = wayland_surface;
+ /* We only need a buffer queue if we have a surface to commit to. */ + if (wws->wayland_surface && !wws->wayland_buffer_queue) + { + wws->wayland_buffer_queue = + wayland_buffer_queue_create(wws->info.bmiHeader.biWidth, + abs(wws->info.bmiHeader.biHeight)); + } + else if (!wws->wayland_surface && wws->wayland_buffer_queue) + { + wayland_buffer_queue_destroy(wws->wayland_buffer_queue); + wws->wayland_buffer_queue = NULL; + } + wayland_window_surface_unlock(window_surface); }
From: Alexandros Frantzis alexandros.frantzis@collabora.com
Track the buffer regions that require an update since the last buffer commit, and copy only those parts from the window_surface to the buffer. --- dlls/winewayland.drv/wayland_surface.c | 12 ++- dlls/winewayland.drv/waylanddrv.h | 18 ++++ dlls/winewayland.drv/window_surface.c | 122 ++++++++++++++++++++++++- 3 files changed, 147 insertions(+), 5 deletions(-)
diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index 43ba853c2c5..13218a977a6 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -33,9 +33,6 @@
WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv);
-/* We only use 4 byte formats. */ -#define WINEWAYLAND_BYTES_PER_PIXEL 4 - /* Protects access to the user data of xdg_surface */ static pthread_mutex_t xdg_data_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -264,6 +261,8 @@ void wayland_shm_buffer_unref(struct wayland_shm_buffer *shm_buffer) wl_buffer_destroy(shm_buffer->wl_buffer); if (shm_buffer->map_data) NtUnmapViewOfSection(GetCurrentProcess(), shm_buffer->map_data); + if (shm_buffer->damage_region) + NtGdiDeleteObjectApp(shm_buffer->damage_region);
free(shm_buffer); } @@ -307,6 +306,13 @@ struct wayland_shm_buffer *wayland_shm_buffer_create(int width, int height, shm_buffer->height = height; shm_buffer->map_size = size;
+ shm_buffer->damage_region = NtGdiCreateRectRgn(0, 0, width, height); + if (!shm_buffer->damage_region) + { + ERR("Failed to create buffer damage region\n"); + goto err; + } + section_size.QuadPart = size; status = NtCreateSection(&handle, GENERIC_READ | SECTION_MAP_READ | SECTION_MAP_WRITE, diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 5abc5ae2d43..992f504e3d7 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -32,11 +32,15 @@
#include "windef.h" #include "winbase.h" +#include "ntgdi.h" #include "wine/gdi_driver.h" #include "wine/rbtree.h"
#include "unixlib.h"
+/* We only use 4 byte formats. */ +#define WINEWAYLAND_BYTES_PER_PIXEL 4 + /********************************************************************** * Globals */ @@ -114,6 +118,7 @@ struct wayland_shm_buffer size_t map_size; BOOL busy; LONG ref; + HRGN damage_region; };
/********************************************************************** @@ -160,6 +165,19 @@ 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;
+/********************************************************************** + * Helpers + */ + +static inline BOOL intersect_rect(RECT *dst, const RECT *src1, const RECT *src2) +{ + dst->left = max(src1->left, src2->left); + dst->top = max(src1->top, src2->top); + dst->right = min(src1->right, src2->right); + dst->bottom = min(src1->bottom, src2->bottom); + return !IsRectEmpty(dst); +} + /********************************************************************** * USER driver functions */ diff --git a/dlls/winewayland.drv/window_surface.c b/dlls/winewayland.drv/window_surface.c index 40129a8977a..94051e8fefd 100644 --- a/dlls/winewayland.drv/window_surface.c +++ b/dlls/winewayland.drv/window_surface.c @@ -205,6 +205,20 @@ out: return shm_buffer; }
+/********************************************************************** + * wayland_buffer_queue_add_damage + */ +static void wayland_buffer_queue_add_damage(struct wayland_buffer_queue *queue, HRGN damage) +{ + struct wayland_shm_buffer *shm_buffer; + + wl_list_for_each(shm_buffer, &queue->buffer_list, link) + { + NtGdiCombineRgn(shm_buffer->damage_region, shm_buffer->damage_region, + damage, RGN_OR); + } +} + /*********************************************************************** * wayland_window_surface_lock */ @@ -254,6 +268,95 @@ static void wayland_window_surface_set_region(struct window_surface *window_surf /* TODO */ }
+/********************************************************************** + * get_region_data + */ +static RGNDATA *get_region_data(HRGN region) +{ + RGNDATA *data; + DWORD size; + + if (!region) return NULL; + if (!(size = NtGdiGetRegionData(region, 0, NULL))) return NULL; + if (!(data = malloc(size))) return NULL; + if (!NtGdiGetRegionData(region, size, data)) + { + free(data); + return NULL; + } + + return data; +} + +/********************************************************************** + * copy_pixel_region + */ +static void copy_pixel_region(char *src_pixels, RECT *src_rect, + char *dst_pixels, RECT *dst_rect, + HRGN region) +{ + static const int bpp = WINEWAYLAND_BYTES_PER_PIXEL; + RGNDATA *rgndata = get_region_data(region); + RECT *rgn_rect; + RECT *rgn_rect_end; + int src_stride, dst_stride; + + if (!rgndata) return; + + src_stride = (src_rect->right - src_rect->left) * bpp; + dst_stride = (dst_rect->right - dst_rect->left) * bpp; + + rgn_rect = (RECT *)rgndata->Buffer; + rgn_rect_end = rgn_rect + rgndata->rdh.nCount; + + for (;rgn_rect < rgn_rect_end; rgn_rect++) + { + char *src, *dst; + int y, width_bytes, height; + RECT rc; + + TRACE("rect %s\n", wine_dbgstr_rect(rgn_rect)); + + if (!intersect_rect(&rc, rgn_rect, src_rect)) continue; + if (!intersect_rect(&rc, &rc, dst_rect)) continue; + + src = src_pixels + rc.top * src_stride + rc.left * bpp; + dst = dst_pixels + rc.top * dst_stride + rc.left * bpp; + width_bytes = (rc.right - rc.left) * bpp; + height = rc.bottom - rc.top; + + /* Fast path for full width rectangles. */ + if (width_bytes == src_stride && width_bytes == dst_stride) + { + memcpy(dst, src, height * width_bytes); + continue; + } + + for (y = 0; y < height; y++) + { + memcpy(dst, src, width_bytes); + src += src_stride; + dst += dst_stride; + } + } + + free(rgndata); +} + +/********************************************************************** + * wayland_window_surface_copy_to_buffer + */ +static void wayland_window_surface_copy_to_buffer(struct wayland_window_surface *wws, + struct wayland_shm_buffer *buffer, + HRGN region) +{ + RECT wws_rect = {0, 0, wws->info.bmiHeader.biWidth, + abs(wws->info.bmiHeader.biHeight)}; + RECT buffer_rect = {0, 0, buffer->width, buffer->height}; + TRACE("wws=%p buffer=%p\n", wws, buffer); + copy_pixel_region(wws->bits, &wws_rect, buffer->map_data, &buffer_rect, region); +} + /*********************************************************************** * wayland_window_surface_flush */ @@ -262,10 +365,12 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) struct wayland_window_surface *wws = wayland_window_surface_cast(window_surface); struct wayland_shm_buffer *shm_buffer = NULL; BOOL flushed = FALSE; + RECT damage_rect; + HRGN surface_damage_region;
wayland_window_surface_lock(window_surface);
- if (IsRectEmpty(&wws->bounds)) goto done; + if (!intersect_rect(&damage_rect, &wws->header.rect, &wws->bounds)) goto done;
if (!wws->wayland_surface || !wws->wayland_buffer_queue) { @@ -277,6 +382,17 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) TRACE("surface=%p hwnd=%p surface_rect=%s bounds=%s\n", wws, wws->hwnd, wine_dbgstr_rect(&wws->header.rect), wine_dbgstr_rect(&wws->bounds));
+ surface_damage_region = NtGdiCreateRectRgn(damage_rect.left, damage_rect.top, + damage_rect.right, damage_rect.bottom); + if (!surface_damage_region) + { + ERR("failed to create surface damage region\n"); + goto done; + } + + wayland_buffer_queue_add_damage(wws->wayland_buffer_queue, surface_damage_region); + NtGdiDeleteObjectApp(surface_damage_region); + shm_buffer = wayland_buffer_queue_get_free_buffer(wws->wayland_buffer_queue); if (!shm_buffer) { @@ -284,7 +400,7 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) goto done; }
- memcpy(shm_buffer->map_data, wws->bits, shm_buffer->map_size); + wayland_window_surface_copy_to_buffer(wws, shm_buffer, shm_buffer->damage_region);
pthread_mutex_lock(&wws->wayland_surface->mutex); if (wws->wayland_surface->current_serial) @@ -300,6 +416,8 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) pthread_mutex_unlock(&wws->wayland_surface->mutex); wl_display_flush(process_wayland.wl_display);
+ NtGdiSetRectRgn(shm_buffer->damage_region, 0, 0, 0, 0); + done: if (flushed) reset_bounds(&wws->bounds); wayland_window_surface_unlock(window_surface);
From: Alexandros Frantzis alexandros.frantzis@collabora.com
When flushing a window_surface, copy from the window_surface only the pixel data contained in the flushed bounds. If any other pixel data are needed, get them from the latest window buffer for the wayland surface, to ensure the data are valid and unchanged. --- dlls/winewayland.drv/wayland_surface.c | 3 ++ dlls/winewayland.drv/waylanddrv.h | 1 + dlls/winewayland.drv/window_surface.c | 54 ++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index 13218a977a6..c770083b23f 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -148,6 +148,9 @@ void wayland_surface_destroy(struct wayland_surface *surface) pthread_mutex_unlock(&surface->mutex); pthread_mutex_unlock(&xdg_data_mutex);
+ if (surface->latest_window_buffer) + wayland_shm_buffer_unref(surface->latest_window_buffer); + wl_display_flush(process_wayland.wl_display);
pthread_mutex_destroy(&surface->mutex); diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 992f504e3d7..909c8ef5b7b 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -107,6 +107,7 @@ struct wayland_surface struct xdg_toplevel *xdg_toplevel; pthread_mutex_t mutex; uint32_t current_serial; + struct wayland_shm_buffer *latest_window_buffer; };
struct wayland_shm_buffer diff --git a/dlls/winewayland.drv/window_surface.c b/dlls/winewayland.drv/window_surface.c index 94051e8fefd..015306bc5f6 100644 --- a/dlls/winewayland.drv/window_surface.c +++ b/dlls/winewayland.drv/window_surface.c @@ -357,6 +357,16 @@ static void wayland_window_surface_copy_to_buffer(struct wayland_window_surface copy_pixel_region(wws->bits, &wws_rect, buffer->map_data, &buffer_rect, region); }
+static void wayland_shm_buffer_copy(struct wayland_shm_buffer *src, + struct wayland_shm_buffer *dst, + HRGN region) +{ + RECT src_rect = {0, 0, src->width, src->height}; + RECT dst_rect = {0, 0, dst->width, dst->height}; + TRACE("src=%p dst=%p\n", src, dst); + copy_pixel_region(src->map_data, &src_rect, dst->map_data, &dst_rect, region); +} + /*********************************************************************** * wayland_window_surface_flush */ @@ -366,7 +376,8 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) struct wayland_shm_buffer *shm_buffer = NULL; BOOL flushed = FALSE; RECT damage_rect; - HRGN surface_damage_region; + HRGN surface_damage_region = NULL; + HRGN copy_from_window_region;
wayland_window_surface_lock(window_surface);
@@ -391,7 +402,6 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) }
wayland_buffer_queue_add_damage(wws->wayland_buffer_queue, surface_damage_region); - NtGdiDeleteObjectApp(surface_damage_region);
shm_buffer = wayland_buffer_queue_get_free_buffer(wws->wayland_buffer_queue); if (!shm_buffer) @@ -400,7 +410,38 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) goto done; }
- wayland_window_surface_copy_to_buffer(wws, shm_buffer, shm_buffer->damage_region); + if (wws->wayland_surface->latest_window_buffer) + { + TRACE("latest_window_buffer=%p\n", wws->wayland_surface->latest_window_buffer); + /* If we have a latest buffer, use it as the source of all pixel + * data that are not contained in the bounds of the flush... */ + if (wws->wayland_surface->latest_window_buffer != shm_buffer) + { + HRGN copy_from_latest_region = NtGdiCreateRectRgn(0, 0, 0, 0); + if (!copy_from_latest_region) + { + ERR("failed to create copy_from_latest region\n"); + goto done; + } + NtGdiCombineRgn(copy_from_latest_region, shm_buffer->damage_region, + surface_damage_region, RGN_DIFF); + wayland_shm_buffer_copy(wws->wayland_surface->latest_window_buffer, + shm_buffer, copy_from_latest_region); + NtGdiDeleteObjectApp(copy_from_latest_region); + } + /* ... and use the window_surface as the source of pixel data contained + * in the flush bounds. */ + copy_from_window_region = surface_damage_region; + } + else + { + TRACE("latest_window_buffer=NULL\n"); + /* If we don't have a latest buffer, use the window_surface as + * the source of all pixel data. */ + copy_from_window_region = shm_buffer->damage_region; + } + + wayland_window_surface_copy_to_buffer(wws, shm_buffer, copy_from_window_region);
pthread_mutex_lock(&wws->wayland_surface->mutex); if (wws->wayland_surface->current_serial) @@ -417,9 +458,16 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) wl_display_flush(process_wayland.wl_display);
NtGdiSetRectRgn(shm_buffer->damage_region, 0, 0, 0, 0); + /* Update the latest window buffer for the wayland surface. Note that we + * only care whether the buffer contains the latest window contents, + * it's irrelevant if it was actually committed or not. */ + if (wws->wayland_surface->latest_window_buffer) + wayland_shm_buffer_unref(wws->wayland_surface->latest_window_buffer); + wayland_shm_buffer_ref((wws->wayland_surface->latest_window_buffer = shm_buffer));
done: if (flushed) reset_bounds(&wws->bounds); + if (surface_damage_region) NtGdiDeleteObjectApp(surface_damage_region); wayland_window_surface_unlock(window_surface); }
From: Alexandros Frantzis alexandros.frantzis@collabora.com
Send the surface damage region to the compositor, to enable it to optimize pixel data transfers. --- dlls/winewayland.drv/wayland_surface.c | 26 +++++++++++++++++++++++--- dlls/winewayland.drv/waylanddrv.h | 5 ++++- dlls/winewayland.drv/window_surface.c | 5 +++-- 3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index c770083b23f..fbc49191d9e 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -226,8 +226,11 @@ void wayland_surface_clear_role(struct wayland_surface *surface) * released by the compositor. */ void wayland_surface_attach_shm(struct wayland_surface *surface, - struct wayland_shm_buffer *shm_buffer) + struct wayland_shm_buffer *shm_buffer, + HRGN surface_damage_region) { + RGNDATA *surface_damage; + TRACE("surface=%p shm_buffer=%p (%dx%d)\n", surface, shm_buffer, shm_buffer->width, shm_buffer->height);
@@ -235,8 +238,25 @@ void wayland_surface_attach_shm(struct wayland_surface *surface, wayland_shm_buffer_ref(shm_buffer);
wl_surface_attach(surface->wl_surface, shm_buffer->wl_buffer, 0, 0); - wl_surface_damage_buffer(surface->wl_surface, 0, 0, - shm_buffer->width, shm_buffer->height); + + /* Add surface damage, i.e., which parts of the surface have changed since + * the last surface commit. Note that this is different from the buffer + * damage region. */ + surface_damage = get_region_data(surface_damage_region); + if (surface_damage) + { + RECT *rgn_rect = (RECT *)surface_damage->Buffer; + RECT *rgn_rect_end = rgn_rect + surface_damage->rdh.nCount; + + for (;rgn_rect < rgn_rect_end; rgn_rect++) + { + wl_surface_damage_buffer(surface->wl_surface, + rgn_rect->left, rgn_rect->top, + rgn_rect->right - rgn_rect->left, + rgn_rect->bottom - rgn_rect->top); + } + free(surface_damage); + } }
/********************************************************************** diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 909c8ef5b7b..0fd2a7a6c77 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -146,7 +146,8 @@ void wayland_surface_destroy(struct wayland_surface *surface) DECLSPEC_HIDDEN; void wayland_surface_make_toplevel(struct wayland_surface *surface) DECLSPEC_HIDDEN; 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) DECLSPEC_HIDDEN; + struct wayland_shm_buffer *shm_buffer, + HRGN surface_damage_region) DECLSPEC_HIDDEN;
/********************************************************************** * Wayland SHM buffer @@ -179,6 +180,8 @@ static inline BOOL intersect_rect(RECT *dst, const RECT *src1, const RECT *src2) return !IsRectEmpty(dst); }
+RGNDATA *get_region_data(HRGN region) DECLSPEC_HIDDEN; + /********************************************************************** * USER driver functions */ diff --git a/dlls/winewayland.drv/window_surface.c b/dlls/winewayland.drv/window_surface.c index 015306bc5f6..baf2c12a922 100644 --- a/dlls/winewayland.drv/window_surface.c +++ b/dlls/winewayland.drv/window_surface.c @@ -271,7 +271,7 @@ static void wayland_window_surface_set_region(struct window_surface *window_surf /********************************************************************** * get_region_data */ -static RGNDATA *get_region_data(HRGN region) +RGNDATA *get_region_data(HRGN region) { RGNDATA *data; DWORD size; @@ -446,7 +446,8 @@ static void wayland_window_surface_flush(struct window_surface *window_surface) pthread_mutex_lock(&wws->wayland_surface->mutex); if (wws->wayland_surface->current_serial) { - wayland_surface_attach_shm(wws->wayland_surface, shm_buffer); + wayland_surface_attach_shm(wws->wayland_surface, shm_buffer, + surface_damage_region); wl_surface_commit(wws->wayland_surface->wl_surface); flushed = TRUE; }
On Tue Jul 4 19:02:35 2023 +0000, Rémi Bernon wrote:
Then changing `wayland_surface_attach_shm` to increase the reference count inside, instead of sinking the reference, which is rather unusual in Wine,
Done (see previous comment).
On Tue Jul 4 19:02:24 2023 +0000, Rémi Bernon wrote:
What about `shm_buffer->busy = FALSE;` here?
In the latest version I have changed the design in the direction you propose, but going on step further, so that the whole "acquisition" (ref+busy) of the buffer is done by the consumer (`wayland_surface_attach_shm`). Among other things, this means that we don't need to perform any cleanup here.
On Wed Jul 5 14:36:45 2023 +0000, Alexandros Frantzis wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/3234/diffs?diff_id=55708&start_sha=c5674d965e1dcc0521a01fbf8c6b271ea51a9a08#3c468ecead08ad532e5c59cf4e59127aa2321329_488_470)
Again, in the latest version, no cleanup is needed.
On Wed Jul 5 14:36:46 2023 +0000, Alexandros Frantzis wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/3234/diffs?diff_id=55708&start_sha=c5674d965e1dcc0521a01fbf8c6b271ea51a9a08#3c468ecead08ad532e5c59cf4e59127aa2321329_474_467)
Changed as suggested. In fact we could have done this before, since the queue itself (which holds a reference to all buffers) and the relevant locking logic guarantees that the `shm_buffer` remains valid.
On Wed Jul 5 14:39:29 2023 +0000, Rémi Bernon wrote:
I only had a quick look so far, but it seems okay already. I wondered if there was a way to use more of NtGdi function to avoid doing the pixel copy by hand, but I don't know very much the API.
I have experimented with using `NtGdi` and pushed the WIP change here: https://gitlab.winehq.org/afrantzis/wine/-/commit/9b396fd07e44ead5373c760ce0....
Although I am not particularly competent with GDI myself, this seems to work OK. I do have some open question about whether this approach will allow us to implement some other [needed pixel operations](https://gitlab.winehq.org/afrantzis/wine/-/blob/wayland/dlls/winewayland.drv...) in the future. I am hoping that with some `NtGdiBitBlt` raster ops with proper brushes and also `NtGdiAlphaBlend` we will be able to get by, but I am not sure yet.
There is a certain elegance going with `NtGdi` for this use case, but at the same time it also seems to be a bit of an overkill :) I think I would be OK switching, with the understanding that we may need to fall back to the hand crafted version, if those future pixel operations prove tricky to implement with `NtGdi`.
On Wed Jul 5 14:39:29 2023 +0000, Alexandros Frantzis wrote:
I have experimented with using `NtGdi` and pushed the WIP change here: https://gitlab.winehq.org/afrantzis/wine/-/commit/9b396fd07e44ead5373c760ce0.... Although I am not particularly competent with GDI myself, this seems to work OK. I do have some open question about whether this approach will allow us to implement some other [needed pixel operations](https://gitlab.winehq.org/afrantzis/wine/-/blob/wayland/dlls/winewayland.drv...) in the future. I am hoping that with some `NtGdiBitBlt` raster ops with proper brushes and also `NtGdiAlphaBlend` we will be able to get by, but I am not sure yet. There is a certain elegance going with `NtGdi` for this use case, but at the same time it also seems to be a bit of an overkill :) I think I would be OK switching, with the understanding that we may need to fall back to the hand crafted version, if those future pixel operations prove tricky to implement with `NtGdi`.
I don't have a strong opinion here, it seems to me that using GDI would be better, and save a bunch of LoC but I have no idea if the future pixel ops can be achieved with it or not. I also don't like much having to keep a section handle open for the lifetime of every window buffer.
Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland_surface.c:
- /* Add surface damage, i.e., which parts of the surface have changed since
* the last surface commit. Note that this is different from the buffer* damage region. */- surface_damage = get_region_data(surface_damage_region);
- if (surface_damage)
- {
RECT *rgn_rect = (RECT *)surface_damage->Buffer;RECT *rgn_rect_end = rgn_rect + surface_damage->rdh.nCount;for (;rgn_rect < rgn_rect_end; rgn_rect++){wl_surface_damage_buffer(surface->wl_surface,rgn_rect->left, rgn_rect->top,rgn_rect->right - rgn_rect->left,rgn_rect->bottom - rgn_rect->top);
If you tell the compositor about the damaged regions, do you still actually need to copy the data outside of it from the previous buffer?
On Wed Jul 5 15:03:14 2023 +0000, Rémi Bernon wrote:
I don't have a strong opinion here, it seems to me that using GDI would be better, and save a bunch of LoC but I have no idea if the future pixel ops can be achieved with it or not. I also don't like much having to keep a section handle open for the lifetime of every window buffer.
I investigated a bit more, but I wasn't able to find a good (and/or efficient) way to implement all the required operations with GDI (ideas are welcome if someone has more experience with GDI).
For example, one operation we will need is: `dst_rgba = src_rgba * alpha`. `NtGdiBitBlt` raster ops don't seem to allow for such color manipulation. `NtGdiAlphaBlend` won't work directly because it blends with the destination using `SRC_OVER`, but in this case we want to disregard the destination and there is no way to adjust the blending function (a workaround would be to first paint the destination region black and then blend with a constant alpha, but that would be inefficient). The closest functionality I could find that would help with this operation is the Color Matrix from GDI+, but GDI+ is not available to the driver.
Another variation we will need is `dst_rgb = src_rgb * alpha; dst_a = alpha;`, which presents us with similar difficulties.
One approach would be to use `NtGdi` for what we can, and fall back to manual pixel manipulation for other cases. However, given that what we can implement with `NtGdi` boils down to simple memcpy operations, this wouldn't help us avoid the extra LoC and complexity, and would, in fact, increase it more since we would now need to maintain both mechanisms.
So, for now, my preference would be to keep the manual implementation, which we know we can enhance with the required operations. If we find a reasonable way to support all we need with `NtGdi` or some other built-in mechanism, I would be happy to switch.
On Thu Jul 6 08:34:38 2023 +0000, Rémi Bernon wrote:
If you tell the compositor about the damaged regions, do you still actually need to copy the data outside of it from the previous buffer?
Sadly, yes, the whole buffer we commit needs to be fully valid/up-to-date, as the compositor is free to read from outside of the damage region as needed. Wayland surface damage is only an optimization hint for the compositor.
On Thu Jul 6 14:18:42 2023 +0000, Alexandros Frantzis wrote:
I investigated a bit more, but I wasn't able to find a good (and/or efficient) way to implement all the required operations with GDI (ideas are welcome if someone has more experience with GDI). For example, one operation we will need is: `dst_rgba = src_rgba * alpha`. `NtGdiBitBlt` raster ops don't seem to allow for such color manipulation. `NtGdiAlphaBlend` won't work directly because it blends with the destination using `SRC_OVER`, but in this case we want to disregard the destination and there is no way to adjust the blending function (a workaround would be to first paint the destination region black and then blend with a constant alpha, but that would be inefficient). The closest functionality I could find that would help with this operation is the Color Matrix from GDI+, but GDI+ is not available to the driver. Another variation we will need is `dst_rgb = src_rgb * alpha; dst_a = alpha;`, which presents us with similar difficulties. One approach would be to use `NtGdi` for what we can, and fall back to manual pixel manipulation for other cases. However, given that what we can implement with `NtGdi` boils down to simple memcpy operations, this wouldn't help us avoid the extra LoC and complexity, and would, in fact, increase it more since we would now need to maintain both mechanisms. So, for now, my preference would be to keep the manual implementation, which we know we can enhance with the required operations. If we find a reasonable way to support all we need with `NtGdi` or some other built-in mechanism, I would be happy to switch.
Alright, thanks.
This merge request was approved by Rémi Bernon.
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Rémi Bernon (＠rbernon)