After some discussion on https://gitlab.winehq.org/wine/wine/-/merge_requests/4533, this refactors how child_window are handled to make it easier to decouple them from the whole_window, and eventually have multiple detached child_window for some Vulkan use case (possibly also GL).
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/window.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 53982bb8c3b..c83b53b6368 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1611,7 +1611,6 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual ) cx = min( max( 1, data->client_rect.right - data->client_rect.left ), 65535 ); cy = min( max( 1, data->client_rect.bottom - data->client_rect.top ), 65535 );
- XSync( gdi_display, False ); /* make sure whole_window is known from gdi_display */ ret = data->client_window = XCreateWindow( gdi_display, data->whole_window ? data->whole_window : dummy_parent, x, y, cx, cy, 0, default_visual.depth, InputOutput, @@ -1698,6 +1697,7 @@ static void create_whole_window( struct x11drv_win_data *data ) sync_window_opacity( data->display, data->whole_window, key, alpha, layered_flags );
XFlush( data->display ); /* make sure the window exists before we start painting to it */ + XSync( gdi_display, False ); /* make sure whole_window is known from gdi_display */
done: if (win_rgn) NtGdiDeleteObjectApp( win_rgn );
From: Rémi Bernon rbernon@codeweavers.com
Making sure it is kept alive as long as the client window lives. --- dlls/winex11.drv/opengl.c | 12 ++++++++++-- dlls/winex11.drv/vulkan.c | 2 +- dlls/winex11.drv/window.c | 10 ++-------- dlls/winex11.drv/x11drv.h | 3 +-- 4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index bb8f13f78b9..329bca11aad 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -224,6 +224,7 @@ struct gl_drawable enum dc_gl_type type; /* type of GL surface */ GLXDrawable drawable; /* drawable for rendering with GL */ Window window; /* window if drawable is a GLXWindow */ + Colormap colormap; /* colormap for the client window */ Pixmap pixmap; /* base pixmap if drawable is a GLXPixmap */ const struct wgl_pixel_format *format; /* pixel format for the drawable */ SIZE pixmap_size; /* pixmap size for GLXPixmap drawables */ @@ -1159,6 +1160,7 @@ static void release_gl_drawable( struct gl_drawable *gl ) TRACE( "destroying %lx drawable %lx\n", gl->window, gl->drawable ); pglXDestroyWindow( gdi_display, gl->drawable ); XDestroyWindow( gdi_display, gl->window ); + XFreeColormap( gdi_display, gl->colormap ); break; case DC_GL_PIXMAP_WIN: TRACE( "destroying pixmap %lx drawable %lx\n", gl->pixmap, gl->drawable ); @@ -1333,7 +1335,10 @@ static struct gl_drawable *create_gl_drawable( HWND hwnd, const struct wgl_pixel NtUserGetAncestor( hwnd, GA_PARENT ) == NtUserGetDesktopWindow()) /* childless top-level window */ { gl->type = DC_GL_WINDOW; - gl->window = create_client_window( hwnd, visual ); + gl->colormap = XCreateColormap( gdi_display, get_dummy_parent(), visual->visual, + (visual->class == PseudoColor || visual->class == GrayScale || + visual->class == DirectColor) ? AllocAll : AllocNone ); + gl->window = create_client_window( hwnd, visual, gl->colormap ); if (gl->window) gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL ); TRACE( "%p created client %lx drawable %lx\n", hwnd, gl->window, gl->drawable ); @@ -1342,7 +1347,10 @@ static struct gl_drawable *create_gl_drawable( HWND hwnd, const struct wgl_pixel else if(usexcomposite) { gl->type = DC_GL_CHILD_WIN; - gl->window = create_client_window( hwnd, visual ); + gl->colormap = XCreateColormap( gdi_display, get_dummy_parent(), visual->visual, + (visual->class == PseudoColor || visual->class == GrayScale || + visual->class == DirectColor) ? AllocAll : AllocNone ); + gl->window = create_client_window( hwnd, visual, gl->colormap ); if (gl->window) { gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL ); diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 101504a7887..469283f336a 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -328,7 +328,7 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance, x11_surface->hwnd = create_info->hwnd; if (x11_surface->hwnd) { - x11_surface->window = create_client_window(create_info->hwnd, &default_visual); + x11_surface->window = create_client_window(create_info->hwnd, &default_visual, default_colormap); x11_surface->hwnd_thread_id = NtUserGetWindowThread(x11_surface->hwnd, NULL); } else diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index c83b53b6368..74cbefd23a7 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1570,7 +1570,7 @@ Window create_dummy_client_window(void) /********************************************************************** * create_client_window */ -Window create_client_window( HWND hwnd, const XVisualInfo *visual ) +Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colormap ) { Window dummy_parent = get_dummy_parent(); struct x11drv_win_data *data = get_win_data( hwnd ); @@ -1595,12 +1595,7 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual ) TRACE( "%p reparent xwin %lx/%lx\n", data->hwnd, data->whole_window, data->client_window ); }
- if (data->client_colormap) XFreeColormap( gdi_display, data->client_colormap ); - data->client_colormap = XCreateColormap( gdi_display, dummy_parent, visual->visual, - (visual->class == PseudoColor || - visual->class == GrayScale || - visual->class == DirectColor) ? AllocAll : AllocNone ); - attr.colormap = data->client_colormap; + attr.colormap = colormap; attr.bit_gravity = NorthWestGravity; attr.win_gravity = NorthWestGravity; attr.backing_store = NotUseful; @@ -1853,7 +1848,6 @@ void X11DRV_DestroyWindow( HWND hwnd ) if (thread_data->last_xic_hwnd == hwnd) thread_data->last_xic_hwnd = 0; if (data->icon_pixmap) XFreePixmap( gdi_display, data->icon_pixmap ); if (data->icon_mask) XFreePixmap( gdi_display, data->icon_mask ); - if (data->client_colormap) XFreeColormap( data->display, data->client_colormap ); free( data->icon_bits ); XDeleteContext( gdi_display, (XID)hwnd, win_data_context ); release_win_data( data ); diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 2917579927c..fb5e07eefbf 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -609,7 +609,6 @@ struct x11drv_win_data Display *display; /* display connection for the thread owning the window */ XVisualInfo vis; /* X visual used by this window */ Colormap whole_colormap; /* colormap if non-default visual */ - Colormap client_colormap; /* colormap for the client window */ HWND hwnd; /* hwnd that this private data belongs to */ Window whole_window; /* X window for the complete window */ Window client_window; /* X window for the client area */ @@ -656,7 +655,7 @@ extern void read_net_wm_states( Display *display, struct x11drv_win_data *data ) extern void update_net_wm_states( struct x11drv_win_data *data ); extern void make_window_embedded( struct x11drv_win_data *data ); extern Window create_dummy_client_window(void); -extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ); +extern Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colormap ); extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ); extern void change_systray_owner( Display *display, Window systray_window ); extern HWND create_foreign_window( Display *display, Window window );
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/window.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 74cbefd23a7..1eedf8523ea 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1549,6 +1549,25 @@ Window get_dummy_parent(void) }
+/********************************************************************** + * detach_client_window + */ +static void detach_client_window( struct x11drv_win_data *data, Window client_window ) +{ + if (data->client_window != client_window || !client_window) return; + data->client_window = 0; + + if (!data->whole_window) return; + + XSelectInput( data->display, client_window, 0 ); + XFlush( data->display ); /* make sure XSelectInput is disabled for client_window after this point */ + XDeleteContext( data->display, client_window, winContext ); + + XReparentWindow( gdi_display, client_window, get_dummy_parent(), 0, 0 ); + TRACE( "%p/%lx detached client window %lx\n", data->hwnd, data->whole_window, client_window ); +} + + /********************************************************************** * create_dummy_client_window */ @@ -1588,12 +1607,7 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colo data->window_rect = data->whole_rect = data->client_rect; }
- if (data->client_window) - { - XDeleteContext( data->display, data->client_window, winContext ); - XReparentWindow( gdi_display, data->client_window, dummy_parent, 0, 0 ); - TRACE( "%p reparent xwin %lx/%lx\n", data->hwnd, data->whole_window, data->client_window ); - } + detach_client_window( data, data->client_window );
attr.colormap = colormap; attr.bit_gravity = NorthWestGravity; @@ -1726,12 +1740,7 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des } else { - if (data->client_window && !already_destroyed) - { - XSelectInput( data->display, data->client_window, 0 ); - XFlush( data->display ); /* make sure XSelectInput doesn't use client_window after this point */ - XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); - } + if (!already_destroyed) detach_client_window( data, data->client_window ); XDeleteContext( data->display, data->whole_window, winContext ); if (!already_destroyed) {
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/opengl.c | 5 ++++- dlls/winex11.drv/vulkan.c | 4 +--- dlls/winex11.drv/window.c | 28 ++++++++++++++++++++++++---- dlls/winex11.drv/x11drv.h | 1 + 4 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 329bca11aad..71888a86d09 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -222,6 +222,7 @@ struct gl_drawable { LONG ref; /* reference count */ enum dc_gl_type type; /* type of GL surface */ + HWND hwnd; GLXDrawable drawable; /* drawable for rendering with GL */ Window window; /* window if drawable is a GLXWindow */ Colormap colormap; /* colormap for the client window */ @@ -1159,7 +1160,7 @@ static void release_gl_drawable( struct gl_drawable *gl ) case DC_GL_CHILD_WIN: TRACE( "destroying %lx drawable %lx\n", gl->window, gl->drawable ); pglXDestroyWindow( gdi_display, gl->drawable ); - XDestroyWindow( gdi_display, gl->window ); + destroy_client_window( gl->hwnd, gl->window ); XFreeColormap( gdi_display, gl->colormap ); break; case DC_GL_PIXMAP_WIN: @@ -1329,6 +1330,7 @@ static struct gl_drawable *create_gl_drawable( HWND hwnd, const struct wgl_pixel gl->refresh_swap_interval = TRUE; gl->format = format; gl->ref = 1; + gl->hwnd = hwnd; gl->mutable_pf = mutable_pf;
if (!known_child && !NtUserGetWindowRelative( hwnd, GW_CHILD ) && @@ -1546,6 +1548,7 @@ void destroy_gl_drawable( HWND hwnd ) { XDeleteContext( gdi_display, (XID)hwnd, gl_hwnd_context ); release_gl_drawable( gl ); + gl->hwnd = 0; } pthread_mutex_unlock( &context_mutex ); } diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 469283f336a..9f0af49b21d 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -214,9 +214,7 @@ static void wine_vk_surface_release(struct wine_vk_surface *surface) pthread_mutex_unlock(&vulkan_mutex); }
- if (surface->window) - XDestroyWindow(gdi_display, surface->window); - + destroy_client_window( surface->hwnd, surface->window ); free(surface); }
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 1eedf8523ea..9abfa01fcc4 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1552,7 +1552,7 @@ Window get_dummy_parent(void) /********************************************************************** * detach_client_window */ -static void detach_client_window( struct x11drv_win_data *data, Window client_window ) +static void detach_client_window( struct x11drv_win_data *data, Window client_window, BOOL reparent ) { if (data->client_window != client_window || !client_window) return; data->client_window = 0; @@ -1563,11 +1563,31 @@ static void detach_client_window( struct x11drv_win_data *data, Window client_wi XFlush( data->display ); /* make sure XSelectInput is disabled for client_window after this point */ XDeleteContext( data->display, client_window, winContext );
- XReparentWindow( gdi_display, client_window, get_dummy_parent(), 0, 0 ); + if (reparent) XReparentWindow( gdi_display, client_window, get_dummy_parent(), 0, 0 ); TRACE( "%p/%lx detached client window %lx\n", data->hwnd, data->whole_window, client_window ); }
+/********************************************************************** + * destroy_client_window + */ +void destroy_client_window( HWND hwnd, Window client_window ) +{ + struct x11drv_win_data *data; + + if (!client_window) return; + + if ((data = get_win_data( hwnd ))) + { + detach_client_window( data, client_window, FALSE ); + release_win_data( data ); + } + + XDestroyWindow( gdi_display, client_window ); + TRACE( "%p destroyed client window %lx\n", hwnd, client_window ); +} + + /********************************************************************** * create_dummy_client_window */ @@ -1607,7 +1627,7 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colo data->window_rect = data->whole_rect = data->client_rect; }
- detach_client_window( data, data->client_window ); + detach_client_window( data, data->client_window, TRUE );
attr.colormap = colormap; attr.bit_gravity = NorthWestGravity; @@ -1740,7 +1760,7 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des } else { - if (!already_destroyed) detach_client_window( data, data->client_window ); + if (!already_destroyed) detach_client_window( data, data->client_window, TRUE ); XDeleteContext( data->display, data->whole_window, winContext ); if (!already_destroyed) { diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index fb5e07eefbf..46900cc505e 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -656,6 +656,7 @@ extern void update_net_wm_states( struct x11drv_win_data *data ); extern void make_window_embedded( struct x11drv_win_data *data ); extern Window create_dummy_client_window(void); extern Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colormap ); +extern void destroy_client_window( HWND hwnd, Window client_window ); extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ); extern void change_systray_owner( Display *display, Window systray_window ); extern HWND create_foreign_window( Display *display, Window window );
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/window.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 9abfa01fcc4..e4f2e051ec9 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1742,7 +1742,7 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des { TRACE( "win %p xwin %lx/%lx\n", data->hwnd, data->whole_window, data->client_window );
- if (data->client_window) XDeleteContext( data->display, data->client_window, winContext ); + detach_client_window( data, data->client_window, !already_destroyed );
if (!data->whole_window) { @@ -1760,7 +1760,6 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des } else { - if (!already_destroyed) detach_client_window( data, data->client_window, TRUE ); XDeleteContext( data->display, data->whole_window, winContext ); if (!already_destroyed) { @@ -1769,7 +1768,7 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des } } if (data->whole_colormap) XFreeColormap( data->display, data->whole_colormap ); - data->whole_window = data->client_window = 0; + data->whole_window = 0; data->whole_colormap = 0; data->wm_state = WithdrawnState; data->net_wm_state = 0;
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/window.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index e4f2e051ec9..97d906587d1 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1568,6 +1568,27 @@ static void detach_client_window( struct x11drv_win_data *data, Window client_wi }
+/********************************************************************** + * attach_client_window + */ +static void attach_client_window( struct x11drv_win_data *data, Window client_window ) +{ + if (data->client_window == client_window || !client_window) return; + detach_client_window( data, data->client_window, TRUE ); + data->client_window = client_window; + + if (!data->whole_window) return; + + XSaveContext( data->display, client_window, winContext, (char *)data->hwnd ); + XSelectInput( data->display, client_window, ExposureMask ); + XFlush( data->display ); /* make sure XSelectInput is enabled for client_window after this point */ + + XReparentWindow( gdi_display, client_window, data->whole_window, data->client_rect.left - data->whole_rect.left, + data->client_rect.top - data->whole_rect.top ); + TRACE( "%p/%lx attached client window %lx\n", data->hwnd, data->whole_window, client_window ); +} + + /********************************************************************** * destroy_client_window */ @@ -1795,7 +1816,6 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) { Window client_window = data->client_window; - Window whole_window = data->whole_window;
if (!data->use_alpha == !use_alpha) return; if (data->surface) window_surface_release( data->surface ); @@ -1803,18 +1823,14 @@ void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BO data->use_alpha = use_alpha;
if (data->vis.visualid == vis->visualid) return; - data->client_window = 0; - destroy_whole_window( data, client_window != 0 /* don't destroy whole_window until reparented */ ); + client_window = data->client_window; + /* detach the client before re-creating whole_window */ + detach_client_window( data, client_window, TRUE ); + destroy_whole_window( data, FALSE ); data->vis = *vis; create_whole_window( data ); - if (!client_window) return; - /* move the client to the new parent */ - XReparentWindow( gdi_display, client_window, data->whole_window, - data->client_rect.left - data->whole_rect.left, - data->client_rect.top - data->whole_rect.top ); - data->client_window = client_window; - XSync( gdi_display, False ); /* make sure XReparentWindow requests have completed before destroying whole_window */ - XDestroyWindow( data->display, whole_window ); + /* attach the client back to the re-created whole_window */ + attach_client_window( data, client_window ); }
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/window.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 97d906587d1..dd6510c94e8 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1632,10 +1632,10 @@ Window create_dummy_client_window(void) */ Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colormap ) { - Window dummy_parent = get_dummy_parent(); struct x11drv_win_data *data = get_win_data( hwnd ); XSetWindowAttributes attr; - Window ret; + unsigned int attr_mask; + Window client_window; int x, y, cx, cy;
if (!data) @@ -1648,38 +1648,32 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual, Colormap colo data->window_rect = data->whole_rect = data->client_rect; }
- detach_client_window( data, data->client_window, TRUE ); - attr.colormap = colormap; attr.bit_gravity = NorthWestGravity; attr.win_gravity = NorthWestGravity; attr.backing_store = NotUseful; attr.border_pixel = 0; + attr_mask = CWColormap | CWBitGravity | CWWinGravity | CWBackingStore | CWBorderPixel;
x = data->client_rect.left - data->whole_rect.left; y = data->client_rect.top - data->whole_rect.top; cx = min( max( 1, data->client_rect.right - data->client_rect.left ), 65535 ); cy = min( max( 1, data->client_rect.bottom - data->client_rect.top ), 65535 );
- ret = data->client_window = XCreateWindow( gdi_display, - data->whole_window ? data->whole_window : dummy_parent, - x, y, cx, cy, 0, default_visual.depth, InputOutput, - visual->visual, CWBitGravity | CWWinGravity | - CWBackingStore | CWColormap | CWBorderPixel, &attr ); - if (data->client_window) + if ((client_window = XCreateWindow( gdi_display, get_dummy_parent(), x, y, cx, cy, 0, default_visual.depth, + InputOutput, visual->visual, attr_mask, &attr ))) { - XSaveContext( data->display, data->client_window, winContext, (char *)data->hwnd ); + XFlush( gdi_display ); /* make sure client_window is created for XSelectInput */ + XSync( data->display, False ); /* make sure client_window is known from data->display */ + + attach_client_window( data, client_window ); XMapWindow( gdi_display, data->client_window ); - if (data->whole_window) - { - XFlush( gdi_display ); /* make sure client_window is created for XSelectInput */ - XSync( data->display, False ); /* make sure client_window is known from data->display */ - XSelectInput( data->display, data->client_window, ExposureMask ); - } + TRACE( "%p xwin %lx/%lx\n", data->hwnd, data->whole_window, data->client_window ); } + release_win_data( data ); - return ret; + return client_window; }
An extra reparent might be expensive, what is the benefit of doing it in two steps?
On Thu Nov 30 18:13:34 2023 +0000, Paul Gofman wrote:
An extra reparent might be expensive, what is the benefit of doing it in two steps?
Using attach_client_window, and having the tiny details (like XSelectInput client window context) handled only within these two (attach / detach) operations.
I don't think it's very expensive compared to all the other operations that we have to do at this point (creating a window + XSync + XFlush + XMapWindow).
-
Paul Gofman (＠gofman) -
Rémi Bernon
-
Rémi Bernon (＠rbernon)